fix(deferred): close backend correctness/efficiency tails from the hygiene sweep
Verified each deferred per-subsystem item against current source, then fixed the
real ones (tradeoffs left as-is, see siftlode-ops/CODE-HYGIENE.md closeout):
- search BUG-3: don't finalise shorts_probed on un-enriched stubs (enrichment
failure/omitted rows) — restores the scheduler's enriched_at guard so a real
Short can't leak into the feed and never get reclassified.
- downloads GC: 4th pass reaps orphaned errored, fileless, unreferenced assets
(they carry no TTL, so the expiry passes never cleared them).
- downloads B6: quota/edit errors now return a structured {code,reason,limit}
the trilingual client localises, instead of hardcoded English + dot-decimal GB.
- channels BB1: reset-backfill re-arms deep sync on every subscriber (bulk update)
instead of 404ing when the admin isn't personally subscribed.
- channels BB2: enrich the stub on BOTH the normal and "already exists" desync
path (nest the insert try inside the client `with`).
- channels CB3: drop the redundant second _discovery_rows read (identity-mapped
rows are already enriched; re-sort in Python for the title tie-break).
- playlists PC1: read the live playlist once in push() and share it with plan_push
+ push_playlist (halves read-quota, closes the second TOCTOU window).
- playlists PC2/PC5: batch the cover thumbnails in one window query (was N+1);
rename combines count+duration into one aggregate + a single cover lookup.
- messages MB2: get_thread only resolves a messageable partner OR one we already
share a thread with (closes the user-enumeration oracle).
- messages MB3: push a live unread-count to the user's other tabs after mark-read.
- messages MC4: list_conversations uses DISTINCT ON + grouped COUNT instead of
loading the whole message history into memory.
- config AB3: per-spec allow_empty so smtp_user/youtube_api_proxy can be blanked
to disable them rather than snapping back to the env default.
This commit is contained in:
npeter83
parent
182dddf5ed
commit
4e80e2b39b
8 changed files with 205 additions and 102 deletions
|
|
@ -175,15 +175,17 @@ def discover_channels(
|
|||
|
||||
rows = _discovery_rows(db, user)
|
||||
|
||||
# Enrich stub channels' metadata (uses the API key — no write scope needed). Re-read
|
||||
# the rows afterwards so the response carries the fresh subscriber counts/thumbnails.
|
||||
# Enrich stub channels' metadata (uses the API key — no write scope needed). The Channel
|
||||
# objects are identity-mapped, so apply_channel_details mutates the very rows we already hold
|
||||
# (fresh subscriber counts/thumbnails carry through without a re-query). Only the title can
|
||||
# change, which affects the tie-break order → re-sort in Python instead of re-hitting the DB.
|
||||
need = [ch.id for ch, _, _ in rows if ch.details_synced_at is None][:DISCOVERY_ENRICH_CAP]
|
||||
if need:
|
||||
try:
|
||||
with quota.attribute(user.id, quota.QuotaAction.CHANNELS_DISCOVER), YouTubeClient(db, user) as yt:
|
||||
apply_channel_details(db, yt.get_channels(need))
|
||||
db.commit()
|
||||
rows = _discovery_rows(db, user)
|
||||
rows = sorted(rows, key=lambda r: (-int(r[1]), (r[0].title or "").lower()))
|
||||
except YouTubeError as exc:
|
||||
log.warning("discovery: channel enrich failed (user %s): %s", user.id, exc)
|
||||
db.rollback()
|
||||
|
|
@ -418,11 +420,18 @@ def reset_backfill(
|
|||
channel = db.get(Channel, channel_id)
|
||||
if channel is None:
|
||||
raise HTTPException(status_code=404, detail="Unknown channel")
|
||||
sub = _user_subscription(db, user, channel_id)
|
||||
channel.backfill_done = False
|
||||
channel.backfill_cursor = None
|
||||
channel.recent_synced_at = None
|
||||
sub.deep_requested = True
|
||||
# Re-arm the demand-driven deep scheduler for this channel. run_deep_backfill gates on an
|
||||
# EXISTS over subscriptions with deep_requested=True, so flip it on EVERY subscriber rather
|
||||
# than requiring the admin to be personally subscribed (they reset from the manager, not
|
||||
# their own feed). No-op row count if the channel has no subscribers.
|
||||
db.execute(
|
||||
update(Subscription)
|
||||
.where(Subscription.channel_id == channel_id)
|
||||
.values(deep_requested=True)
|
||||
)
|
||||
db.commit()
|
||||
with quota.attribute(user.id, quota.QuotaAction.VIDEOS_BACKFILL_RECENT):
|
||||
run_recent_backfill(db, [channel], max_channels=1)
|
||||
|
|
@ -455,25 +464,30 @@ def subscribe(
|
|||
).scalar_one_or_none()
|
||||
if existing is not None:
|
||||
raise HTTPException(status_code=409, detail="Already subscribed to this channel.")
|
||||
try:
|
||||
with quota.attribute(user.id, quota.QuotaAction.CHANNELS_SUBSCRIBE), YouTubeClient(db, user) as yt:
|
||||
with quota.attribute(user.id, quota.QuotaAction.CHANNELS_SUBSCRIBE), YouTubeClient(db, user) as yt:
|
||||
try:
|
||||
yt_sub_id = yt.insert_subscription(channel_id)
|
||||
# Enrich a stub channel (title/thumbnail/subscriber count) so it shows up
|
||||
# properly right away; no video pull here.
|
||||
if channel.details_synced_at is None:
|
||||
except YouTubeError as exc:
|
||||
# Already following on YouTube but not recorded here (a desync — e.g. a stale import
|
||||
# dropped the local row, so it resurfaced in discovery). That's not a failure: the end
|
||||
# state the user wanted already holds, so record it locally and move on. The next
|
||||
# subscription resync fills in the resource id. Any other YouTube error is a real
|
||||
# problem — surface it with a clear message (422, not a 502 that the client would
|
||||
# mistake for a transient "connection lost").
|
||||
msg = str(exc).lower()
|
||||
if "already exists" in msg or "duplicate" in msg:
|
||||
yt_sub_id = ""
|
||||
else:
|
||||
raise HTTPException(status_code=422, detail=f"YouTube couldn't subscribe you: {exc}")
|
||||
# Enrich a stub channel (title/thumbnail/subscriber count) so it shows up properly right
|
||||
# away, on BOTH the normal and the desync path — a resurfaced stub still needs metadata to
|
||||
# render. Keep the client open (we're still inside the `with`); a metadata failure here
|
||||
# must not block recording the subscription the user asked for.
|
||||
if channel.details_synced_at is None:
|
||||
try:
|
||||
apply_channel_details(db, yt.get_channels([channel_id]))
|
||||
except YouTubeError as exc:
|
||||
# Already following on YouTube but not recorded here (a desync — e.g. a stale import
|
||||
# dropped the local row, so it resurfaced in discovery). That's not a failure: the end
|
||||
# state the user wanted already holds, so record it locally and move on. The next
|
||||
# subscription resync fills in the resource id. Any other YouTube error is a real
|
||||
# problem — surface it with a clear message (422, not a 502 that the client would
|
||||
# mistake for a transient "connection lost").
|
||||
msg = str(exc).lower()
|
||||
if "already exists" in msg or "duplicate" in msg:
|
||||
yt_sub_id = ""
|
||||
else:
|
||||
raise HTTPException(status_code=422, detail=f"YouTube couldn't subscribe you: {exc}")
|
||||
except YouTubeError as exc:
|
||||
log.warning("subscribe: stub enrich failed (%s): %s", channel_id, exc)
|
||||
db.add(
|
||||
Subscription(
|
||||
user_id=user.id,
|
||||
|
|
|
|||
|
|
@ -265,7 +265,7 @@ def enqueue_download(
|
|||
db, user.id, source_kind, source_ref, spec, profile_id, display_name
|
||||
)
|
||||
except quota.QuotaExceeded as e:
|
||||
raise HTTPException(status_code=422, detail=_quota_message(e))
|
||||
raise HTTPException(status_code=422, detail=_quota_detail(e))
|
||||
return _serialize_job(db, job)
|
||||
|
||||
|
||||
|
|
@ -288,27 +288,16 @@ def enqueue_edit(
|
|||
try:
|
||||
job = service.enqueue_edit(db, user.id, source_job, spec, display_name)
|
||||
except quota.QuotaExceeded as e:
|
||||
raise HTTPException(status_code=422, detail=_quota_message(e))
|
||||
raise HTTPException(status_code=422, detail=_quota_detail(e))
|
||||
except service.EditError as e:
|
||||
raise HTTPException(status_code=400, detail=_edit_message(e))
|
||||
raise HTTPException(status_code=400, detail={"code": "edit", "reason": e.reason})
|
||||
return _serialize_job(db, job)
|
||||
|
||||
|
||||
def _edit_message(e: service.EditError) -> str:
|
||||
if e.reason == "empty_edit":
|
||||
return "Choose a trim range or crop area first."
|
||||
if e.reason == "source_not_ready":
|
||||
return "The source download isn't ready to edit."
|
||||
return "That edit couldn't be created."
|
||||
|
||||
|
||||
def _quota_message(e: quota.QuotaExceeded) -> str:
|
||||
if e.reason == "max_jobs":
|
||||
return f"You've reached your download limit ({e.limit} items). Remove some first."
|
||||
if e.reason == "max_bytes":
|
||||
gb = e.limit / 1_073_741_824
|
||||
return f"You've used your download storage quota ({gb:.1f} GB). Free up space first."
|
||||
return "Download quota exceeded."
|
||||
def _quota_detail(e: quota.QuotaExceeded) -> dict:
|
||||
"""Structured error the trilingual client localizes (reason key + numbers), instead of a
|
||||
pre-baked English string with a dot-decimal GB value. See lib/api.ts localizeDetail()."""
|
||||
return {"code": "quota", "reason": e.reason, "limit": e.limit, "current": e.current}
|
||||
|
||||
|
||||
@router.get("")
|
||||
|
|
|
|||
|
|
@ -17,7 +17,7 @@ from datetime import datetime, timezone
|
|||
|
||||
from fastapi import APIRouter, Depends, HTTPException, WebSocket, WebSocketDisconnect
|
||||
from pydantic import BaseModel
|
||||
from sqlalchemy import and_, func, or_, select
|
||||
from sqlalchemy import and_, case, func, or_, select
|
||||
from sqlalchemy.orm import Session
|
||||
|
||||
from app.auth import require_human, resolved_user_id
|
||||
|
|
@ -220,25 +220,40 @@ def list_conversations(
|
|||
with the latest message + my unread count, newest first. Lazily creates the welcome on the
|
||||
first open. User-message previews are ciphertext — the client decrypts them."""
|
||||
ensure_welcome(db, user)
|
||||
msgs = (
|
||||
# The conversation partner for a message, from this user's perspective (system → 0).
|
||||
partner_expr = case(
|
||||
(Message.kind == "system", SYSTEM_PARTNER_ID),
|
||||
(Message.sender_id == user.id, Message.recipient_id),
|
||||
else_=Message.sender_id,
|
||||
)
|
||||
mine = or_(Message.sender_id == user.id, Message.recipient_id == user.id)
|
||||
# Latest message per conversation in ONE query (DISTINCT ON, Postgres) instead of loading the
|
||||
# user's whole message history into Python — bounded by the number of conversations.
|
||||
latest = (
|
||||
db.execute(
|
||||
select(Message)
|
||||
.where(or_(Message.sender_id == user.id, Message.recipient_id == user.id))
|
||||
.order_by(Message.created_at.desc())
|
||||
.where(mine)
|
||||
.distinct(partner_expr)
|
||||
.order_by(partner_expr, Message.created_at.desc())
|
||||
)
|
||||
.scalars()
|
||||
.all()
|
||||
)
|
||||
convs: dict[int, dict] = {}
|
||||
for m in msgs:
|
||||
# Unread incoming, grouped per conversation.
|
||||
unread = dict(
|
||||
db.execute(
|
||||
select(partner_expr, func.count())
|
||||
.where(mine, Message.recipient_id == user.id, Message.read_at.is_(None))
|
||||
.group_by(partner_expr)
|
||||
).all()
|
||||
)
|
||||
|
||||
def _pid(m: Message) -> int:
|
||||
if m.kind == "system":
|
||||
pid = SYSTEM_PARTNER_ID
|
||||
else:
|
||||
pid = m.recipient_id if m.sender_id == user.id else m.sender_id
|
||||
c = convs.setdefault(pid, {"last": m, "unread": 0})
|
||||
if m.recipient_id == user.id and m.read_at is None:
|
||||
c["unread"] += 1
|
||||
user_ids = [pid for pid in convs if pid != SYSTEM_PARTNER_ID]
|
||||
return SYSTEM_PARTNER_ID
|
||||
return m.recipient_id if m.sender_id == user.id else m.sender_id
|
||||
|
||||
user_ids = [_pid(m) for m in latest if m.kind != "system"]
|
||||
partners = {}
|
||||
if user_ids:
|
||||
partners = {
|
||||
|
|
@ -246,7 +261,8 @@ def list_conversations(
|
|||
for u in db.execute(select(User).where(User.id.in_(user_ids))).scalars().all()
|
||||
}
|
||||
items = []
|
||||
for pid, c in convs.items():
|
||||
for m in latest:
|
||||
pid = _pid(m)
|
||||
if pid == SYSTEM_PARTNER_ID:
|
||||
partner = _system_partner()
|
||||
elif pid in partners:
|
||||
|
|
@ -254,7 +270,7 @@ def list_conversations(
|
|||
else:
|
||||
continue
|
||||
items.append(
|
||||
{"partner": partner, "last_message": _serialize_msg(c["last"]), "unread": c["unread"]}
|
||||
{"partner": partner, "last_message": _serialize_msg(m), "unread": int(unread.get(pid, 0))}
|
||||
)
|
||||
items.sort(key=lambda x: x["last_message"]["created_at"] or "", reverse=True)
|
||||
return {"items": items}
|
||||
|
|
@ -275,9 +291,6 @@ def get_thread(
|
|||
cond = and_(Message.recipient_id == user.id, Message.kind == "system")
|
||||
else:
|
||||
p = db.get(User, partner_id)
|
||||
if p is None:
|
||||
raise HTTPException(status_code=404, detail="User not found")
|
||||
partner = _serialize_user(p)
|
||||
cond = and_(
|
||||
Message.kind == "user",
|
||||
or_(
|
||||
|
|
@ -285,6 +298,13 @@ def get_thread(
|
|||
and_(Message.sender_id == partner_id, Message.recipient_id == user.id),
|
||||
),
|
||||
)
|
||||
# MB2: don't leak arbitrary user identities. Resolve a partner only if they're messageable
|
||||
# OR we already share a thread (so a conversation with a since-suspended/deactivated user
|
||||
# still opens). Otherwise the same 404 as a non-existent id → no enumeration oracle.
|
||||
has_history = db.scalar(select(Message.id).where(cond).limit(1))
|
||||
if p is None or (not is_messageable_user(p) and not has_history):
|
||||
raise HTTPException(status_code=404, detail="User not found")
|
||||
partner = _serialize_user(p)
|
||||
msgs = (
|
||||
db.execute(select(Message).where(cond).order_by(Message.created_at.asc()))
|
||||
.scalars()
|
||||
|
|
@ -299,6 +319,15 @@ def get_thread(
|
|||
changed = True
|
||||
if changed:
|
||||
db.commit()
|
||||
# MB3: tell this user's OTHER tabs/devices their unread dropped so the badge updates
|
||||
# live instead of waiting for the next 20s poll. Carries the fresh total so the client
|
||||
# needs no extra fetch.
|
||||
n = db.scalar(
|
||||
select(func.count())
|
||||
.select_from(Message)
|
||||
.where(Message.recipient_id == user.id, Message.read_at.is_(None))
|
||||
)
|
||||
manager.push(user.id, {"type": "unread", "count": int(n or 0)})
|
||||
return {"partner": partner, "items": [_serialize_msg(m) for m in msgs]}
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -92,19 +92,12 @@ def _remove_item(db: Session, pl: Playlist, video_id: str, *, mark_dirty: bool)
|
|||
|
||||
|
||||
def _summary(
|
||||
db: Session,
|
||||
pl: Playlist,
|
||||
count: int,
|
||||
total_duration: int = 0,
|
||||
has_video: bool | None = None,
|
||||
cover: str | None = None,
|
||||
) -> dict:
|
||||
cover = db.scalar(
|
||||
select(Video.thumbnail_url)
|
||||
.join(PlaylistItem, PlaylistItem.video_id == Video.id)
|
||||
.where(PlaylistItem.playlist_id == pl.id)
|
||||
.order_by(PlaylistItem.position)
|
||||
.limit(1)
|
||||
)
|
||||
out = {
|
||||
"id": pl.id,
|
||||
"name": pl.name,
|
||||
|
|
@ -121,18 +114,6 @@ def _summary(
|
|||
return out
|
||||
|
||||
|
||||
def _total_duration(db: Session, playlist_id: int) -> int:
|
||||
return (
|
||||
db.scalar(
|
||||
select(func.coalesce(func.sum(Video.duration_seconds), 0))
|
||||
.select_from(PlaylistItem)
|
||||
.join(Video, Video.id == PlaylistItem.video_id)
|
||||
.where(PlaylistItem.playlist_id == playlist_id)
|
||||
)
|
||||
or 0
|
||||
)
|
||||
|
||||
|
||||
@router.get("")
|
||||
def list_playlists(
|
||||
contains: str | None = None,
|
||||
|
|
@ -183,13 +164,34 @@ def list_playlists(
|
|||
.scalars()
|
||||
.all()
|
||||
)
|
||||
# Batch the cover thumbnails (first item per playlist by position) in ONE window query
|
||||
# instead of a per-playlist SELECT inside _summary (the old N+1).
|
||||
rn = func.row_number().over(
|
||||
partition_by=PlaylistItem.playlist_id,
|
||||
order_by=PlaylistItem.position,
|
||||
).label("rn")
|
||||
first_items = (
|
||||
select(
|
||||
PlaylistItem.playlist_id.label("pid"),
|
||||
Video.thumbnail_url.label("thumb"),
|
||||
rn,
|
||||
)
|
||||
.join(Video, Video.id == PlaylistItem.video_id)
|
||||
.where(PlaylistItem.playlist_id.in_(ids))
|
||||
.subquery()
|
||||
)
|
||||
covers = dict(
|
||||
db.execute(
|
||||
select(first_items.c.pid, first_items.c.thumb).where(first_items.c.rn == 1)
|
||||
).all()
|
||||
)
|
||||
return [
|
||||
_summary(
|
||||
db,
|
||||
p,
|
||||
counts.get(p.id, 0),
|
||||
durations.get(p.id, 0),
|
||||
(p.id in member) if contains else None,
|
||||
covers.get(p.id),
|
||||
)
|
||||
for p in pls
|
||||
]
|
||||
|
|
@ -207,7 +209,7 @@ def create_playlist(
|
|||
pl = Playlist(user_id=user.id, name=name, position=_next_playlist_position(db, user.id))
|
||||
db.add(pl)
|
||||
db.commit()
|
||||
return _summary(db, pl, 0)
|
||||
return _summary(pl, 0)
|
||||
|
||||
|
||||
def _watch_later(db: Session, user: User) -> Playlist:
|
||||
|
|
@ -350,13 +352,23 @@ def push(
|
|||
)
|
||||
try:
|
||||
with quota.attribute(user.id, quota.QuotaAction.PLAYLISTS_PUSH):
|
||||
plan = plan_push(db, user, pl)
|
||||
# Read the live playlist ONCE (linked playlists only) and share it with both the
|
||||
# plan and the push, halving read-quota and closing the second read→write TOCTOU
|
||||
# window. A fresh export (no yt_playlist_id) reads nothing.
|
||||
live = None
|
||||
if pl.yt_playlist_id:
|
||||
with YouTubeClient(db, user) as yt:
|
||||
live = (
|
||||
list(yt.iter_playlist_items_with_ids(pl.yt_playlist_id)),
|
||||
yt.get_playlist_snippet(pl.yt_playlist_id),
|
||||
)
|
||||
plan = plan_push(db, user, pl, live=live)
|
||||
if plan["units_estimate"] > quota.remaining_today(db):
|
||||
raise HTTPException(
|
||||
status_code=429,
|
||||
detail="Not enough YouTube quota left today for this sync.",
|
||||
)
|
||||
return push_playlist(db, user, pl)
|
||||
return push_playlist(db, user, pl, live=live)
|
||||
except YouTubeError:
|
||||
db.rollback()
|
||||
raise HTTPException(status_code=422, detail="YouTube playlist sync failed.")
|
||||
|
|
@ -405,10 +417,25 @@ def rename_playlist(
|
|||
pl.name = name
|
||||
recompute_dirty(db, pl)
|
||||
db.commit()
|
||||
count = db.scalar(
|
||||
select(func.count()).where(PlaylistItem.playlist_id == pl.id)
|
||||
# One aggregate for count + duration (outer join so an empty playlist still returns 0),
|
||||
# plus a single cover lookup — was 3 separate round-trips.
|
||||
count, total = db.execute(
|
||||
select(
|
||||
func.count(PlaylistItem.id),
|
||||
func.coalesce(func.sum(Video.duration_seconds), 0),
|
||||
)
|
||||
.select_from(PlaylistItem)
|
||||
.outerjoin(Video, Video.id == PlaylistItem.video_id)
|
||||
.where(PlaylistItem.playlist_id == pl.id)
|
||||
).one()
|
||||
cover = db.scalar(
|
||||
select(Video.thumbnail_url)
|
||||
.join(PlaylistItem, PlaylistItem.video_id == Video.id)
|
||||
.where(PlaylistItem.playlist_id == pl.id)
|
||||
.order_by(PlaylistItem.position)
|
||||
.limit(1)
|
||||
)
|
||||
return _summary(db, pl, count or 0, _total_duration(db, pl.id))
|
||||
return _summary(pl, count or 0, total or 0, cover=cover)
|
||||
|
||||
|
||||
@router.delete("/{playlist_id}")
|
||||
|
|
|
|||
|
|
@ -60,6 +60,11 @@ def _classify_shorts(db: Session, videos: list[Video]) -> None:
|
|||
for v in videos:
|
||||
if v.shorts_probed:
|
||||
continue
|
||||
if v.enriched_at is None:
|
||||
# Enrichment failed (proxy blip) or YouTube omitted the row — duration is unknown,
|
||||
# so DON'T finalise shorts_probed off a null duration; leave it for the scheduler
|
||||
# (which only classifies enriched rows) to retry. Otherwise a real Short would leak.
|
||||
continue
|
||||
if (
|
||||
v.live_status != "none"
|
||||
or v.duration_seconds is None
|
||||
|
|
@ -164,7 +169,13 @@ def _ingest_candidates(
|
|||
# 4) Confirm/deny Shorts (no quota) so none enter via search.
|
||||
_classify_shorts(db, videos)
|
||||
|
||||
keep = {v.id for v in videos if not v.is_short and v.live_status not in LIVE_OR_UPCOMING}
|
||||
keep = {
|
||||
v.id
|
||||
for v in videos
|
||||
if v.enriched_at is not None
|
||||
and not v.is_short
|
||||
and v.live_status not in LIVE_OR_UPCOMING
|
||||
}
|
||||
return [vid for vid in ids if vid in keep]
|
||||
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue