Round three, on round two's diff. The trend across the loop is 47 -> 34 -> 15 raw
findings; the ops half kept producing the sharpest ones because the gate cannot run it.
**`docker compose start` ignores a newly pinned IMAGE_TAG.** Round two removed the
automatic restart because it re-ran `alembic upgrade head` — and then told the operator to
run that exact command by hand, in both scripts and the docs. `start` relaunches the
container that already exists, which still carries the tag being rolled back FROM, so
pinning 0.79.0 and following the printed instruction brings 0.80.0 back up and re-applies
its migrations. Verified against a real daemon: after changing the pinned tag, `start` kept
the old image and only `up -d` picked up the new one. Both scripts and the doc now say
`up -d`, with the pin-first caveat where the operator reads it.
**The reset flag outlived its token.** Reverting round one's "each link clears the other"
went one step too far: a `#verify=` link pasted over a standing reset notice replaces the
whole fragment, so the reset token is gone — but `resetPending` stayed true, and being
checked first it hid the confirmation while promising a recovery that signing out could no
longer complete. The flag is now cleared when the fragment it describes is gone, which is
narrower than what was reverted and does not blank the signed-out card.
**A test that could not fail.** The shared-asset regression test seeded the asset as
`pending`, and `_reset_asset_pending` only acts on `downloading` — so the release half was a
no-op and deleting it kept the suite green. Seeded as the worker actually leaves it now.
Also: the malformed-URL guard answers with urlguard's structured detail rather than a bare
English string, so both halves of one typo class get the same localized copy, and the third
site in that module handing user input to `urlparse` was swept with it; the asset is
released after the job is terminal, not before; the Stats 403 toast gets the wizard handle
its own comment claimed it had; the pasted Windows recipe sets `$ErrorActionPreference`
inside the block (a scriptblock inherits the caller's, so a `SilentlyContinue` profile
swallowed every throw) and cleans up the container-side dump on the failure path too; the
restore warning moved to the warning stream so a `> log` redirect cannot file it away; and
two comments narrowed to what the code does.
Gate green: vitest 685, pytest 950.
Round two of the review loop, on round one's own diff. Two of its findings were
regressions the fixes themselves introduced, which is exactly what the loop is for:
**A per-user refusal was writing onto a shared row.** MediaAssets are deduped across
users, and the generic failure handler puts the message on the asset — right for an
upstream failure, wrong for "your storage is full". Two users queuing the same video share
one asset, so the first user hitting their cap failed the second user's job with the first
one's private reason, telling someone with an empty library to free up space. The refusal
now raises `_OwnerRefused`, caught separately: the job fails, the asset goes back to
`pending` for whoever else holds it. Pinned by a test that drives `_download` directly —
the refusal happens before extraction, so nothing needs stubbing.
**The restore scripts restarted the app, undoing the restore.** `docker compose start`
re-runs the container command, and that runs `alembic upgrade head` — so on the rollback
path this documents as recommended (restore the dump, THEN pin the older tag) the newer
image immediately re-applied the migrations the restore had just undone, and on a FAILED
restore it put the app back on a half-restored schema. Both are the opposite of why the
stop was added. Nothing is restarted automatically now; the scripts say what to run and
when, and the docs' snippets say the same.
**Reverted rather than patched:** the OnboardingWizard toast (the wizard already renders
that failure inline, next to a Retry button — three recoveries for one failure), and
`useVerifyEntry`'s "each new link clears the other". The latter blanked the signed-out
card's only banner, where nothing renders the reset flag to replace it. The two states are
independent again; what changes is that `dismiss` peels ONE layer, which is what the
original finding was actually about.
Also: `Stats` uses the YouTube helper rather than the bare one, so the route's English 403
detail no longer beats the translated fallback under a Hungarian UI; `urlparse` guarded
alongside `.port` (an unclosed bracket raised just as late), the reason renamed `malformed`
since "bad port" misdescribes it; three docstrings narrowed to what the code actually does
(the worker's byte check is per-job, not an aggregate ledger, and not on the edit path;
the redirect residual leaks a reachability oracle, which the note had denied; the
invalidation helper does not cover suspend); the pasted Windows recipe wrapped so `throw`
can stop it; the bash one-liner's failure branch no longer exits 0; and the tests that were
missing for the round-one fixes — the account panel's error paths, the invalidation set,
and the shared-asset case above.
Gate green: vitest 685, pytest 950.
The deep review of the branch raised 47 findings, 18 of which survived adversarial
verification. Grouped by what they actually were:
**The size ceiling inverted at its boundary.** `remaining_bytes` clamped to 0 for a user
already at their storage cap, and `build_ydl_opts` reads 0 as "no limit" — so the one
user over quota was the only one downloading with no ceiling at all. `check_enqueue`
could not catch it: it ran while the job was queued and the user was still under the cap.
The arithmetic moves into `quota.remaining_bytes` (where every other max_bytes decision
lives, and where it can be tested), and the worker now fails the job outright rather than
proceeding unbounded — as a non-retryable `quota_exceeded`, since retrying cannot help
until space is freed.
**Silent failures in the new account panel.** All three actions went through `quiet: true`
endpoints, which suppress the global error dialog on the promise that the caller reports
inline — with no `onError`, that promise was broken and a refusal rendered as nothing. On
the broken-SMTP instance this panel exists for, an admin would read silence as "the mail
went out". The co-invalidation set and the email-off predicate move to `lib/adminUsers`,
shared with the pending-access queue, so the two copies of these mutations cannot drift
apart again — which is how half of this was lost in the first place.
**A rollback recipe that destroyed data without working.** `alembic downgrade -1` steps
back one migration, but a release routinely ships several (0.80.0 shipped four): it drops
the newest one's columns and leaves the schema at a revision the older image also cannot
resolve, i.e. the crash loop it was meant to escape, minus the data. The dump restore is
now the recommended path, and the alembic route asks the target version for its own head
revision instead of counting backwards.
Also: redirects named in the urlguard docstring beside the DNS-rebinding residual they
share a fix with (SEC-17); a malformed port refused as `bad_port` instead of escaping as
a 500; the pull cooldown extended to the sibling route spending the same QuotaAction;
restore stops the app before dropping its tables; the PowerShell cleanup can no longer
decide the script's verdict on 5.1, nor report the wrong exit code; the documented
one-liners hardened to match the scripts; the registry's retention window stated where
pinning is advertised; the two banners made mutually exclusive so neither can shadow the
other; `hover:bg-hover` (a utility this project never generated) replaced by the
`selectable` recipe; the clipboard helper reused; and the sync cooldown's copy swept to
the two callers that discarded it.
Verified: gate green (vitest 678, pytest 949), and the panel, the row hover and the reset
banner UAT'd on localdev.
SEC-12: the download box accepts an arbitrary http(s) URL and yt-dlp fetches
it from the WORKER, so any approved user could aim the server at the compose
network, the LAN or a cloud metadata endpoint — and read the result off
job.error, where "connection refused" vs "HTTP Error 401" vs "timed out" is a
port scanner. downloads/urlguard.py refuses a URL whose host resolves to a
loopback/private/link-local/reserved address, at enqueue AND again in the
worker (DNS can move in between, and the worker is where the socket opens).
A network-class failure on a `url` job now reports one fixed sentence instead
of the raw transport text; the error CODE is still classified from the
original, so the localized explanation is unchanged. DNS rebinding is
explicitly NOT closed — that needs the connection gated inside yt-dlp — and
the module says so rather than implying more than it does.
SEC-13: `noplaylist` only demotes a video-inside-a-playlist URL to that video,
so a URL that is purely a playlist or channel still expanded into every
entry — one enqueue, unbounded downloads. `playlist_items` pins it to one.
Size was unbounded too: `check_enqueue` compares the CURRENT footprint against
the cap, and a download's size is unknown until it lands, so one job could
overshoot by any amount; the owner's remaining budget is now handed to yt-dlp
as `max_filesize`, applied AFTER the adapter merge so no source adapter can
raise a limit the quota layer set. A join's cut-list is capped at 100: an
accurate join emits two filters per segment into one ffmpeg filter_complex.
SEC-14: the YouTube budget is instance-wide, so one user holding down a
manual sync spent the day for everyone. Per-user (not per-IP — the cost
belongs to the account) 3-per-10-minutes limiters on the two bulk pulls,
subscriptions and playlists, with separate budgets so one does not close the
other. The review also listed /rss, /backfill and /enrich; E3 deleted them,
and search stays unlimited on purpose — it is interactive, not a bulk pull.
Gate: pytest 896 -> 924 (none skipped), vitest 671.
Deep-review round 1 on bug/download-mediakind-probe found a real HIGH the prior
review missed:
- resolve_media_facts used a per-field `probe or info` fallback, so a probe that
RAN and deliberately found no video track (cover-art-only audio: attached_pic
skipped; a still image: format-guarded) had its None video overridden by
yt-dlp's stale vcodec. An audio-mode download from a progressive source (info
carries the source's video codec, embed-thumbnail on) was then classified VIDEO
-> a Play button + a broken in-app <video> with no video track. Only the
download path regressed (upload calls probe_media directly).
Fix: gate precedence on whether ffprobe RAN, not per-field. If the probe read
the file, its codec verdict wins outright (a None means 'no such track'); only a
full ffprobe failure falls back to info. Dimensions still gap-fill from info.
Refactored the parsing into _facts_from_probe so resolve_media_facts can see the
raw probe-ran signal; probe_media keeps its 5-tuple (upload + TestProbeClassifier
unchanged).
- tests: the cover-art-audio and still-image resurrection cases, a differing
populated-info precedence case (the prior-flagged gap), and the ffprobe-failed
info fallback — now stubbing run_ffprobe_json so the real parsing + precedence
run together.
Backend gate green (ruff + full pytest).
A downloaded video was silently demoted to a download-only "file" (no
in-app play/edit) whenever yt-dlp's info dict omitted vcodec/acodec — which
it does for Facebook and other sources. Finalize derived media_kind from
those nulls (`derive(None, None)` -> file), so a genuine H.264/AAC MP4
became non-playable in the library.
Fix: probe the finished file on disk (the ground truth) at finalize and
derive from that, falling back to the info dict only for a field the probe
couldn't read. This is the same ffprobe the upload ingest already used, now
extracted to a shared downloads/probe.py so the download worker and the
upload path share one classifier (and can't drift). Also backfills the
stored vcodec/acodec/width/height from the real file.
Tests: TestResolveMediaFacts pins probe-wins-over-info (incl. the Facebook
no-codec case and yt-dlp's literal "none"); upload's probe tests repoint to
the shared module.
Deep-review remediation for the E7 upload path, all findings adversarially verified:
- Serve every library file with a Content-Type chosen from the ffprobe-verified media_kind
+ a strict container allow-list (storage.served_media_type), never guessed from the
user-controlled file extension. Closes a stored-XSS vector where a media/HTML polyglot
uploaded as "evil.html" was served inline as text/html on the login-free /watch surface.
Applied to both the authenticated /file and the public watch_file endpoints.
- Classify uploaded still images (jpg/png/gif/webp/…) as media_kind='file' so they are
force-downloaded and don't get a broken in-app video player.
- Roll back the on-disk side of an upload when the ingest transaction fails after place_file
(unlink the placed file + its deterministic poster path), so a failed commit can't orphan
bytes in the GC-skipped .uploads tree.
- Floor download_upload_max_bytes at min=1 so a stored 0 can't silently disable the per-file cap.
- Dedupe the ffprobe-JSON invocation into edit.run_ffprobe_json (shared by probe_codecs and
upload._probe) and the upload extension into storage._clean_upload_ext; drop the now-unused
parse_probe_streams.
Tests: public + authenticated force-download and the polyglot-not-served-as-HTML guard, the
real _probe classifier (image/cover-art/dims/duration), the ingest rollback/cleanup path, the
max_jobs branch, the cap-floor invariant, and run_ffprobe_json error handling.
Add the non-yt-dlp ingest path so a user-uploaded file joins the same download
library and inherits its function set. Any file type is accepted: media
(video/audio) gets the full set (metadata, share, play/edit), a generic file
(pdf/zip/docx/…) is download-only.
- MediaAsset.media_kind ("video"|"audio"|"file") — the stored discriminator that
gates library actions. Set at every finalize (download probe, editor carries the
source's kind, upload probe); consolidates the ad-hoc audio-detect that lived only
in links.py into one invariant (downloads.mediakind.derive). Migration 0076 adds
the column + backfills existing rows.
- downloads/upload.py: ingest_upload probes the file (ffprobe → codecs/dims/duration),
places it in a per-user .uploads tree, and creates an already-ready asset + done job
(the same cache-hit shortcut enqueue uses). Uploads never expire (irreplaceable).
- POST /api/downloads/upload: streaming multipart with the per-file cap
(download_upload_max_bytes, default 500 MiB) enforced on actual bytes read; quota
charges the known size up front (quota.check_upload).
- Security: a generic file is always served force-download (attachment +
application/octet-stream) on both the in-app and public endpoints, so uploaded
HTML/SVG can't execute inline. Edit + redownload reject non-media / upload sources.
- python-multipart added (first multipart endpoint in the app).
The Download module's purpose is download -> share -> play in-browser, so the
output must play in EVERY browser (iOS/Safari/WebKit included). The only
universally-safe combo is mp4 + H.264 + AAC/MP3.
- formats.normalize() now CLAMPS every spec to that profile (container->mp4,
vcodec->None, audio_format opus->m4a) at the single point all paths flow
through (picker, stored profile, raw API) — not just by hiding UI options. A
source exposing no H.264 is still caught by the worker's browser-safe
re-encode, so playability is guaranteed end-to-end.
- build_ydl_opts drops the now-unreachable non-compat branches (+ _VCODEC_SORT).
- _SIG_VERSION 3->4 so assets cached under an old non-compat spec — or a stale
non-H.264 mp4 that slipped the v3 net (the HEVC Facebook dls) — re-derive.
- ProfileEditor drops the container/codec pickers and Opus; audio is M4A/MP3.
- i18n: remove the now-unused container/vcodec/any keys (HU+EN parity).
Each user stores their own login cookies (Netscape cookies.txt) for an
auth-walled download host (Facebook, Instagram, X, TikTok, Reddit) in
Settings; a download of an auth-walled link on that host is retried with
the acting user's cookie.
Two load-bearing security controls, both in downloads.credentials as the
single source of truth so enqueue and the worker can't drift:
- cache privacy: a cookie-authed download salts its MediaAsset format_sig
with the user id, so the authed asset is private and never cache-served
to another user. The worker attaches a cookie ONLY when the asset it is
downloading into IS that private, salted identity.
- host scope (SEC-12): a cookie is resolved only for a URL whose netloc
the credential owns (netloc-scoped _host_is); a look-alike/unrelated
host gets none.
cookies_enc is Fernet-encrypted at rest, only decrypted in-worker into a
0600 file under staging/.auth/ (survives the retry wipe + _produced_file
scan) and unlinked in finally. The blob is never returned by a route,
logged, audited, or written to a job's error/snapshot. cookiefile is set
after the adapter's extra merge so an adapter can't clobber it. Settings
UI is write-only (host + set/updated only, never echoed). New
download_credentials table + migration 0074; per-user routes; HU+EN i18n.
Tests: host-scope, cache-privacy (no cross-user serve), encrypt-at-rest,
write-only API, worker attach-gate; FE dom test for the write-only field.
The download/edit child (yt-dlp + ffmpeg) shares one LAN box with the API,
Postgres, and Plex playback. A CPU re-encode pegged every core for ~30s and
starved web requests + playback until it finished. Full-scope S4 mitigations:
- Child self-de-prioritizes at startup (inherited by every ffmpeg/yt-dlp
descendant): CPU nice increment + best-effort idle ionice class + an
OMP_NUM_THREADS budget, all config-gated and independently disable-able.
- Cap the browser-safe re-encode's encoder threads (`-threads`) so a transcode
reserves a core for foreground work instead of claiming all of them.
- Throttle the retention GC's whole-tree orphan sweep (yield IO after each
batch of files) so its rglob can't monopolize disk IO in the API process.
- Idle-back-off the Download Center (2s→15s when nothing queued/running) and
UsageBar (2s→15s when running_jobs is 0) polls; both snap back to 2s on the
next fetch after an enqueue (invalidateDownloads refetches immediately).
Tests: encode_thread_cap (pin/reserve-core/edge), -threads flag presence,
_deprioritize (nice/ionice/OMP branches), GC sweep throttle batching.
5a — the public watch page rendered every asset in a <video>, but WebKit (all
iOS browsers) refuses to play an audio-only mp3 in <video>, so shared audio
links were broken on iPad while working on desktop Chromium. public_meta now
flags audio-only assets (codec present, no video stream) and WatchPage renders
a native <audio> in a padded surface card (no artwork exists for these).
5c — a login-walled Facebook post failed as a bare 'unknown', which offered a
misleading Retry. Classify closed-group / cookie-suggestion auth walls as
login_required and genuinely unsupported sites as a new unsupported_host class
(both non-retryable), and drop unknown from the modal's retryable set — an
unclassified failure is most often an unparsed auth wall. The queue row keeps
its own force-retry for real transients.
Round-2 review of the R1 fixes found that the two-independent-limiter progress
writer issued a redundant heartbeat every interval during an ACTIVE download: a
percent change writes progress (which already refreshes updated_at, the stall
watchdog's liveness source), then the sub-integer calls that follow fire the
separate beat budget — ~2 DB writes/interval instead of the throttle's intended 1.
Replace the second limiter with a shared `last_touch` clock (last DB write OR
beat). Progress writes keep their own `allow_write` budget (so a beat still can't
delay a real write — the R1 concern), but the heartbeat now fires only when
nothing touched the row for a full interval. Active download → ~1 write/interval;
a genuinely stuck re-encode → 1 heartbeat/interval. New regression test:
an unchanged-percent call in the same interval as a write does NOT heartbeat.
ruff clean, full DB-lane 365 pass.
Round-1 review fixes on the E3 branch:
- prune partial-fetch (correctness): `_paginate` now reports whether it reached
the server's reported `total`; `_prune_stale` prunes only on a COMPLETE
non-empty listing. A truncated fetch (a mid-sequence empty/short page before
total — a transient Plex hiccup) no longer deletes the rows it merely failed
to list. New test: a listing that claims 5 but returns 1 prunes nothing.
- `_prune_stale` (robustness): `stats.get("pruned", 0) + 1` — no KeyError when a
caller's stats dict omits the key.
- service transitions (conventions): pause/resume/redownload use the JobStatus
StrEnum instead of loose "queued"/"paused"/"error" literals, matching cancel.
- ffmpeg progress writer (correctness): a stuck-percent heartbeat now runs on its
OWN 1/interval budget, so it can't consume the progress-write slot and delay a
real percent change by up to an interval.
- backend/app/README.md (conventions): reword the transaction-ownership rule to
quote and extend CLAUDE.md's canonical ("handler/job-wrapper commits; helpers
flush; only quota owns its session") instead of stating a contradicting
"helpers MAY commit" carve-out.
ruff clean, full DB-lane 364 pass.
worker.py (~1090 L) mixed the PARENT supervisor (claim + supervise + the process
loop) with the CHILD pipeline (the yt-dlp/ffmpeg download+edit that runs in the
spawned subprocess). Extract them:
worker.py 1090 -> 475 parent: claim/supervise/loop
downloads/pipeline.py (new 592) the child pipeline (_download/_process_edit +
hooks/progress/ffmpeg helpers + _Aborted)
downloads/jobdb.py (new 73) the short-lived-session helpers both sides
share (_set_job/_job_status/_touch_job/
_requeue_if_running/_reset_asset_pending +
the staging-dir paths)
The graph is one-directional (runner -> pipeline -> jobdb; worker -> jobdb; no
worker<->pipeline edge) — the parent never calls the child directly, it spawns
runner.py as a subprocess. Consumers rewired: runner import, and two tests follow
the moved symbols (_media_info/_produced_file -> pipeline; the ffmpeg-callback
patches target pipeline where the names are looked up).
Pure code motion, verbatim bodies. ruff clean, full lane 363 pass (unchanged).
pause/resume/cancel/delete/redownload each hand-rolled their asset bookkeeping
inline in the route (release, failed-asset reset, the 20-line fresh-asset
rebuild), duplicating logic the worker/GC also touch. Move each transition into a
service.* method (service.pause/resume/cancel/delete/redownload) that owns the
mutation + its commit; the routes keep only auth, the HTTP 409 guards, and
serialization. Drops the now-dead _set_status route helper and two stale imports.
Pure behaviour-preserving code motion (routes -56 lines). Green under the E3 S2
transition net + the full downloads suite (363 pass), ruff clean.
MediaAsset.ref_count (how many jobs hold a shared cache asset) was a Python
read-modify-write at four sites (enqueue, enqueue_edit, release_asset, the
redownload route). Two concurrent enqueues of the same (source,format) both read
the same value and write the same +1 → the count ends up one short → the first
release drives it to zero and deletes the file out from under a live holder.
Replace all four with atomic DB ops: incr_ref_count (`ref_count = ref_count + 1`)
and decr_ref_count (`greatest(ref_count - 1, 0)` RETURNING the new value, so the
release decides whether to free the asset from the post-decrement count while
holding the row's write lock — Postgres serialises concurrent releases on that
lock, closing the double-free). Pure DB-atomicity change, no semantic shift.
Green under the E3 S2 ref_count net (5) + full lane (356 pass), ruff clean.
Review round on the whole E1 stack (main..HEAD). 6 findings, all low/medium,
none blocking; fixed for a clean gate-green handoff.
- plex/sync (correctness): the un-enriched cast guard compared the stored cast
against THIS listing's length, so a trimmed Plex cast on a not-yet-enriched
row was never applied. Compare against the listing CAP (_LISTING_CAST_CAP=20)
instead: >cap = enriched (keep, survives a watermark reset), <=cap =
listing-sourced (track edits in both directions). +2 regression tests.
- main (security): add X-Frame-Options: DENY as the legacy-browser fallback for
the CSP frame-ancestors clickjacking guard.
- downloads/service (altitude): finalize_if_running now uses a SQLAlchemy 2.0
update() construct instead of a hand-built text() UPDATE string — same atomic
WHERE status='running', idiomatic, no identifier interpolation, no manual
StrEnum coercion. Drops the now-unused text import.
- auth/admin (reuse): extract the SEC-10 "drop untrusted preset password + bump
epoch" invariant into one helper (revoke_preset_password_if_unverified) so the
Google-callback and admin-activation paths can't drift.
- tests/migrations (robustness): guarantee the shared schema is restored to head
in a finally, so a downgrade/re-upgrade failure can't cascade into later tests.
- worker (simplification): align _finish_from_cache's display_name fill with the
_download path's guard (no behavior change).
Gate green: ruff clean, 334 pytest (0 skip), frontend unchanged.
Follow-up to the /code-review high on the S1 branch. The per-site
`if job.status == 'running'` guards were a non-atomic read-then-write (TOCTOU):
a cancel committing between the child's status read and its commit could still
be overwritten with done/error, re-opening the double-release window.
Introduce service.finalize_if_running(db, job_id, **fields): one atomic
`UPDATE download_jobs SET ... WHERE status='running'` that returns whether it
fired. It flushes pending asset writes first so the asset-then-job row-lock
order matches the cancel route (no deadlock), and runs in the caller's session
so it commits with the asset writes in one transaction. All five worker sites
(download/edit completion, both error paths, cache finish) now go through it,
so the invariant lives in one place. The 'job done' logs move inside the
finalized branch so a job stopped mid-completion no longer logs a false 'done'.
Add tests/test_downloads_finalize.py: running -> finalizes; canceled/paused ->
no-op.
The cancel route writes status 'canceled' but the supervisor compared against
'cancelled', so its "a route stopped it -> kill the child, free the asset"
branch was dead code. A canceled job's child ran to completion and wrote 'done'
unconditionally over the route's 'canceled'; since 'done' holds the asset but
'canceled' does not, a later delete released the asset a second time, dropping
ref_count below the live holder count and deleting a file still in use.
Add app/downloads/status.py as the single source of truth for the lifecycle
literals (JobStatus StrEnum + the HOLDING / STOPPED_BY_ROUTE sets) and use it at
the safety-critical sites: the supervisor stop check, the route cancel, and
release_asset's holding set. Make the worker's terminal done/error writes
conditional on the row still being 'running' so a child finishing just after a
route cancel/pause can no longer resurrect it. Add the missing 'canceled'
supervisor regression test.
(The full literal sweep across the downloads surface is left for the E3
downloads owner-unification; this is the data-safety subset.)
- Floor the effective max_concurrent at 1 on the read path (quota.resolve)
and in the claim SQL (GREATEST(..., 1)). admin_set_quota already floors on
write and the sysconfig default's min is 1, but a stray stored 0 (direct DB
edit) would diverge the two "at limit" checks — the SQL claim-window
exclusion can't see a user "capped" with zero running jobs, while
at_concurrency_limit can — reopening the starvation the fairness fix closes.
Keeping the effective limit >= 1 makes the two checks provably agree.
- shared_with_me now orders by (created_at DESC, id DESC) to match
list_downloads, so the newly-added 500 cap has a stable boundary among
same-timestamp shares instead of a non-deterministic one.
- 0062 upgrade loop: rename the unused loop var "table" to "_table"
(matching the underscore-marked "_col").
Test: a stored max_concurrent=0 flooder no longer wedges the claim loop.
resolve_adapter matched sources by loose substring over the netloc, so a
look-alike host ('myyoutube.com', 'youtube.com.evil') misresolved. Match the
host exactly or as a subdomain via a _host_is() helper, and identify from the
netloc of BOTH source_ref and yt-dlp's webpage_url — the latter by host only,
never a substring over its path. This also restores a host signal to the
early-fill path (resolve_adapter(None, None, info)), which has no source_ref,
so a Facebook asset's uploader_url no longer depends on extractor_key being
present in the progress-hook info_dict.
Tests: look-alike hosts stay generic while real subdomains resolve; a
webpage_url netloc identifies the source when extractor_key is absent.
- resolve_adapter: identify from the yt-dlp extractor_key only, not the full
webpage_url (a sibling source's name in a URL path no longer misresolves);
add youtube-nocookie.com to the YouTube host match so nocookie embeds keep
the POT/player-client extraction strategy.
- _download: re-resolve the adapter with the post-extraction info dict so the
.nfo uniqueid type and uploader_url agree on one source (dedups the inline
resolve too).
- MediaMeta.uniqueid_type defaults to source-neutral "generic" instead of
"youtube", so an un-updated caller can never falsely label a non-YouTube
download as youtube.
- Tests: lock extractor-key-only resolution + nocookie host; default nfo type.
write_sidecars / delete_asset_files computed `base = media.with_suffix("")` and then
`base.with_suffix(".nfo")`. Stripping the real `.mp4` first leaves a stem whose own last dot may
be inside the title (e.g. "Sonka Házilag."), which `with_suffix` then treats as the extension and
clobbers — writing the .nfo/.jpg under a TRUNCATED basename that no longer matches the media, so
Plex/Kodi never associate them (and delete_asset_files targets the wrong name, leaking orphans).
Swap the extension on the FULL media path via a shared `_sidecar` helper instead.
Pre-existing bug (predates the source-adapters work); found during its E2E and folded in here.
Verified end-to-end: a dotted-title Facebook reel now writes a single shared basename.
Consolidates source-specific policy (youtube / facebook / reddit / generic) that was scattered
across worker.py, backfill.py, service.py, formats.py and storage.py with no shared source
identity. app/downloads/adapters.py holds the base (= generic yt-dlp behaviour) + per-source
overrides; resolve_adapter(source_kind, source_ref, info) picks one by source_kind, the yt-dlp
extractor_key, or the URL host, falling back to generic. yt-dlp stays the engine — the adapter is
policy only.
Moved into adapters (behaviour-equivalent):
- uploader_url derivation (YouTube channel_url / Facebook uploader_id / reddit None) — removes the
worker/backfill duplication (build_uploader_url + _should_refetch gone).
- the player-client fallback sets + retriable markers + YouTube extractor_args/POT (build_ydl_opts
now takes source-agnostic opts plus an `extra` dict from the adapter).
- source_url reconstruction (service.source_url delegates to the adapter).
Two intentional (approved) behaviour changes:
- the player-client fallback is now YouTube-only; non-YouTube sources make a single extraction
attempt instead of a pointless double attempt with youtube-only client args.
- the .nfo <uniqueid type=...> follows the source (adapter.nfo_uniqueid_type) instead of the
hardcoded "youtube" that mislabelled every url-source download.
Deliberately left generic (documented non-moves): title cleaning (normalize_title is harmless
across sources), the classify marker table, thumbnail_to_jpg, the format selectors, and og.py's
YouTube-hqdefault og:image preference (a self-contained thumbnail heuristic on the share path).
Verified end-to-end on localdev across all four adapters: downloads succeed and uploader_url + the
.nfo uniqueid type are correct per source.
- worker.py: wrap the watchdog loop so a failure in it (e.g. a transient DB error in
_job_liveness) still hard-kills the child + fails the job, instead of leaving the child
running unsupervised — the exact wedge the feature prevents (#1).
- worker.py: _touch_job heartbeat bumps updated_at at each extraction attempt, so a slow /
multi-client-set fallback extraction (which writes no progress, and re-writes identical
values so ORM onupdate does not fire) is not false-killed as "stalled" (#2).
- edit.py: extract escalate_kill (terminate->wait->kill escalation); _stop and worker's
_stop_child (process-group variant) now share it (#3).
- worker.py: document the edit path's second, tighter run_ffmpeg watchdog and that the
supervisor is POSIX/Linux-container-only (#4, #6).
- tests: a long download stays alive while progress is fresh, and a watchdog-loop error
still stops the child (#1, #5).
Defense-in-depth after the ReDoS incident. The heavy work (yt-dlp download / ffmpeg edit)
now runs in a child process (app.downloads.runner) supervised by the worker thread, not in
the thread itself. A thread can't be killed and a C-level spin can't even be cooperatively
cancelled, so one wedged download used to pin a core, hold the GIL and freeze the whole
queue; a child process can be SIGTERM/SIGKILLed.
The supervisor kills the child (and its process group, so ffmpeg grandchildren die too)
when: a route pauses/cancels the job, it makes no progress for download_stall_timeout_seconds
(150s), it runs past download_wall_clock_max_seconds (2h), the worker is shutting down, or
the child crashes without setting state. On a kill it frees the asset and fails the job with
a retryable network error (stalls are not auto-requeued). A clean exit is left alone — the
child already wrote the terminal state, including a cooperative pause via the progress hook.
- config.py: stall / wall-clock / supervisor-poll / kill-grace settings.
- worker.py: _run_supervised + _spawn_download_child / _job_liveness / _stop_child /
_cleanup_staging; _process_job claims the asset, then hands the heavy work to the child.
- app/downloads/runner.py: the child entrypoint.
- docker-compose.localdev.yml: init:true on the worker so tini (PID 1) reaps orphaned
ffmpeg grandchildren left by a hard-kill (the worker reaps its own direct children).
- tests: watchdog decision logic + SIGTERM->SIGKILL escalation (fake proc, no real subprocess).
- edit.py: extract _run_ffmpeg_best_effort (one run+timeout+swallow policy shared by
thumbnail_to_jpg / ensure_poster / ensure_storyboard) + a build_thumbnail_jpg_cmd
builder, matching the module's other ffmpeg commands.
- thumbnail_to_jpg: copy an already-JPEG thumbnail (Facebook, and Reddit's jpeg-under-
a-.png-name) instead of re-encoding it; only webp/png-proper are decoded via ffmpeg.
- errors.py: drop the redundant "account authentication" classify marker
("authentication is required" already covers the known Reddit message).
- formats.py: document that the opt-in EmbedThumbnail is unaffected by dropping the
convertor (it converts unsupported thumbs to PNG, not the crashing mjpeg2jpeg path).
- test_titles.py: guard the ReDoS regression with a thread + timeout (deterministic
"must not hang") instead of a wall-clock assertion.
Three prod-observed download bugs, root-caused from a frozen prod queue with the
worker pinned at 100% CPU:
- ReDoS in normalize_title: _TRAILING_HASHTAGS had two overlapping unbounded
quantifiers around the required letter, so a Facebook title with a long hashtag
cluster that is NOT at the end ("... #a #b #c | Page") backtracked catastrophically.
It runs inside the yt-dlp progress hook holding the GIL, so one such download froze
the whole worker (and could stall feed enrichment too - sync/videos.py calls it).
Rewrite with a zero-width "has a letter" lookahead + a possessive tag body: one
unambiguous split, nothing to backtrack. 40-tag stress case: ~0.1ms.
- Reddit download killed by a cosmetic thumbnail convert: yt-dlp's
FFmpegThumbnailsConvertor runs "-bsf:v mjpeg2jpeg" on a JPEG-served-under-a-.png-name
(Reddit) and fails; being a postprocessor, it failed the WHOLE download even though
the media was fine. Drop it and produce the <base>.jpg poster/og:image ourselves via
editmod.thumbnail_to_jpg (plain ffmpeg decode->mjpeg, best-effort -> ensure_poster
cuts a frame on failure). The FB Messenger / MS Teams share preview keeps working
(posters verified as real JPEGs).
- classify: map "authentication is required" (Reddit's auth wall) to login_required
instead of unknown, so the UI explains it rather than offering a pointless retry.
Tests: ReDoS-timing + numeric-marker regression, the Reddit classify case, and
thumbnail_to_jpg (real JPEG out / None on bad input, never raises).
The R12 S3 underscore-publish left worker.uploader_url() sharing a name with the
`uploader_url` model attribute and the yt-dlp `"uploader_url"` info-dict key. The
function is now build_uploader_url() — the attribute, dict key, log line and
docstrings keep the plain `uploader_url` name. Rename touches only the def, its
two call sites, and the backfill import; no behaviour change (ruff clean,
pytest 193).
Layering cleanup, no behaviour change (ruff clean backend+scripts, pytest 193):
- Invert the scheduler->routes dependency: reset_demo_state (+ its demo-seed
helpers) move from routes/admin.py to a new service module app/demo.py.
Both routes/admin.py and scheduler.py now import it from app.demo, so the
scheduler no longer reaches into the routes layer — its lazy import (the
routes<->scheduler cycle workaround) is gone, hoisted to a top-level import.
- Publish the private symbols that were imported across module boundaries (the
underscore convention was lying): feed._serialize -> serialize,
feed._to_tsquery_str -> to_tsquery_str, videos._insert_stubs -> insert_stubs,
videos._stub_from_playlist_item -> stub_from_playlist_item,
worker._uploader_url -> uploader_url, plex._shared._item_or_404 ->
item_or_404. Scoped renames — the 5 other route modules keep their own truly
private _serialize helpers.
- _release_asset moves from routes/downloads.py to app/downloads/service.py as
the public release_asset (asset bookkeeping is a service concern); the routes
call it via service.release_asset.
Facebook's scraper rejected our self-hosted poster as "Corrupted Image": the poster is an
ffmpeg-produced JPEG (non-standard marker order) that browsers/iOS/Twitter decode fine but FB's
stricter processor won't. For a YouTube source, point og:image at the video's stable, non-expiring
`i.ytimg.com/vi/<id>/hqdefault.jpg` (a Google-encoded JPEG FB decodes reliably) built from the id in
the stored thumbnail — instead of the stored `sqp`-signed variant, which can expire. Non-YouTube
sources keep the poster fallback (fine everywhere but FB). Declares 480×360 so FB includes it on the
first async scrape. og.py only — no schema change.
- og: guard safe_abs_path's None (missing/gc'd poster) before _jpeg_size, else the public /watch OG
render crashed with an uncaught AttributeError
- channels: hoist the priority step button to module scope so it no longer remounts on every
hold-repeat tick (which dropped the pointer capture and could let a hold run away on pointer drift)
- watch: only route audio through Web Audio on iOS (where a backgrounded <video> is suspended);
other platforms keep their reliable native audio, avoiding a foreground-silence risk if the
AudioContext can't resume
- share: drop the unstable `create` mutation object from the auto-create effect deps (it changes
every render); the links.data trigger + autoTried ref already fire it exactly once
The clipboard-on-focus finding is intended (the user explicitly asked for auto-paste on focus; the
read is best-effort and browsers reject it without a gesture, so no real prompt fires).
The FB Sharing Debugger confirmed the real cause of the missing desktop-Messenger preview: the
scraper fetches og:image ASYNCHRONOUSLY on a URL's first scrape ("The provided 'og:image'
properties are not yet available… specify the dimensions using 'og:image:width'/'og:image:height'"),
so a freshly shared /watch link unfurled title-only. Read the poster JPEG's real dimensions from its
SOF marker (Pillow isn't a dep; the poster aspect differs from the video's, so the stored video
width/height won't do) and emit og:image:width/height. Only runs on a crawler hit. Unit-tested.
The iPad Messenger app unfurls a share link on-device (fetches + renders the OG tags itself),
but messenger.com desktop shows only what Facebook's server-side scraper cached — which is
pickier. Add og:image:secure_url (public HTTPS) and og:image:type=image/jpeg for our poster so
the card renders even when the scraper won't sniff the bytes. Reachability/stale-cache is a
separate, live-diagnosis step.
Failed downloads stored the raw yt-dlp/ffmpeg text and dumped it as a truncated red
one-liner in the queue. Classify the failure into a category (unavailable / login_required /
geo_blocked / format_unavailable / drm / postprocess / source_missing / network / unknown)
and surface it properly:
- backend: app/downloads/errors.classify maps the raw message to a category code; the worker
stores it in the new download_jobs.error_code (migration 0061) at both failure sites; resume
clears it; the serializer exposes it. Raw text kept as technical detail.
- frontend: the queue row shows a clean localized reason + a "details" link opening a modal
with the explanation, the raw text behind a disclosure, and a Retry only for transient
categories (network/postprocess/unknown).
- tests: classify() unit coverage over verbatim yt-dlp/ffmpeg message shapes.
- run_ffmpeg stops the process from ONE exit path (`except BaseException`), so
an exception out of on_progress (a DB write) can no longer orphan a live
ffmpeg after a 10s join on its pipes; _stop is now a no-op on a dead process
- both run_ffmpeg callbacks are throttled to ~1s (they were a DB round-trip per
output line: ~10^5 queries on a long re-encode), mirroring the download hook
- volume arithmetic extracted to lib/playerVolume with unit tests; a corrupt
level now falls back to the LAST GOOD one instead of full blast, and the
sanitising happens where the stored value enters
- RSS is back to two except clauses (the shared tail already sits after the try)
- new tests: callback-exception cleanup, the two throttles; the fake Popen grew
poll() and its wait count is now asserted
Mutation-checked: each new group goes red when its fix is reverted.
- HLS sweep only deletes directories matching the session-name shape; the root
is admin-configurable and may be a shared path
- playlist unwrap picks the first entry that actually downloaded, not entries[0]
- concat quoting keeps backslashes literal (ffmpeg treats them so inside '')
- ArrowUp/Down defer while the focused element can still scroll that way
- the staging fallback skips yt-dlp working files (.part-Frag/.ytdl/.temp/.fNNN)
- the boot HLS sweep runs off the event loop
- volume persistence is debounced (was a setItem per wheel notch) and a spin no
longer loses notches to the iframe's lagging getVolume
- _Breaker.run owns the whole open/call/record protocol; four call sites shrink
- should_prune extracted and unit-tested, plus RSS error-vs-empty tests
- the worker exits non-zero when the schema never lands
ffprobe prints codec_name BEFORE codec_type, so the line parser attributed every
name to the previous stream: a plain H.264+AAC mp4 probed as (aac, None) and the
browser-playability check re-encoded a perfectly good file on EVERY download.
Verified on localdev — the same source now stores without a re-encode pass.
- pick the produced file safely: Path("") is Path("."), which passes .exists(),
so a missing yt-dlp filepath skipped the staging fallback and crashed in
with_name ("PosixPath('.') has an empty name") on non-YouTube sources
- unwrap a playlist/multi_video extraction to the entry that carries the media
- run_ffmpeg: drain stderr while it runs (a full 64KB pipe blocked ffmpeg
forever and hung the worker thread), 1s cancel polling, stall watchdog and a
bounded wait on exit
- escape apostrophes/backslashes in the ffmpeg concat list (pre-0.51.0 names)
- sweep orphaned HLS segment dirs at startup (nothing else ever reaped them)
- C-3.7: fold a per-link password_version into the signed watch grant (migration
0059 adds the column) and bump it on every password change, so rotating a share
link's password invalidates outstanding grants at once instead of letting them
live out the 6h TTL. make_grant/check_grant take the version; update_link bumps.
- C-3.8: the share recipient picker and share-by-email now reuse the messageable
filter (not-demo AND active AND not-suspended) instead of only excluding demo,
so suspended/deactivated addresses aren't leaked or reachable as targets.
- C-3.9: GET /keys/{user_id} adopts get_thread's MB2 guard — a non-messageable
target with no shared history returns the same "User not found" as a missing id,
so it can't be probed to enumerate users or fetch suspended users' keys.
- C-3.12: _register_account catches IntegrityError on the insert (the select-then-
insert TOCTOU) and treats it as the already-registered no-op.
- C-3.13: trigger_job claims the running flag atomically under _activity_lock
(compare-and-set) instead of a bare check, closing the double-start window.
- Move _messageable/is_messageable_user into a shared app/userscope.py so downloads
doesn't import the messages route module.
Follow-up to user prod testing of 0.51.0:
- re-download now clears job.display_name so the card title + Content-Disposition filename are
re-derived from the freshly-cleaned title (previously an auto-name from an older un-cleaned
title — still carrying a "… views · … reactions |" prefix — survived the rebuild).
- on-disk AND served names are now a strict shell-safe slug: only [A-Za-z0-9._-] survive; every
other char (the "!" the user saw, plus ( ) [ ] & $ ' " ; | ? * …) collapses to "_". The id no
longer uses [brackets] (glob metacharacters). Shared _slug helper for sanitize + display_filename.
- GC gains a 5th pass: sweep on-disk files/dirs no ready asset owns (old accented channel dirs, a
poster from a superseded naming scheme left after a re-download), skipping the dot system trees
and anything newer than an hour so an in-flight download is never touched.
- Tests: slug strips !/$/()[]; rel_path is bracket-free; orphan-sweep keeps owned/system/fresh files.
- redownload now ALWAYS rebuilds into a fresh private asset instead of ever reusing a shared
cached file: the cache sig is salted with job_id + current asset_id, so the fork is distinct
from both the dedup pool and the job's own current asset. This resolves the co-held silent
no-op (finding #1) — a shared asset is left intact for its other holders while THIS job
re-fetches its own copy. Resolving the fork BEFORE releasing the old hold (and the unique
salted key) also removes the IntegrityError-rollback-after-file-delete window (finding #2).
- format_sig gains an optional `salt` for that fork identity.
- titles: don't consume the space after `|`, so a "stats | #hashtags" remainder keeps the
leading whitespace _TRAILING_HASHTAGS needs — and fall back to the prefix+hashtag-stripped
text, so neither the stats nor the SEO hashtags are resurrected when a title empties (#3).
- worker: early-fill asset.title falls back to the video id so a title that cleaned to empty
doesn't show a blank card while downloading (#5).
- Tests for the salt fork and the hashtag-remainder case.
(Finding #4 — quota on re-download — intentionally not changed: a re-download reuses the
existing job and re-fetches ~the same bytes, so calling check_enqueue would wrongly reject on
the job-count cap while footprint is unchanged.)
- redownload: never clobber a co-held (deduped) asset. Drop the force-reset-to-pending;
_release_asset already deletes+recreates a solo asset (genuine re-fetch), while a shared
ready asset is left intact and the re-queued job re-points to it via _finish_from_cache —
so one user's re-download can't delete another's file mid-serve (finding #1).
- redownload: reset queue_pos to the tail so a re-download doesn't front-run newer queued
jobs on the worker's `ORDER BY queue_pos` claim (finding #3). Made service.next_queue_pos public.
- formats: video-only mp4 selector prefers every video-only option before any progressive
`best`, so a "Video only" download stays audio-free whenever a video-only stream exists (#2).
- titles: a title that is only the engagement prefix no longer resurrects the stripped stats
(return the stripped text / empty, not the raw) (#4).
- worker: probe the file's duration when yt-dlp gave none, so a long re-encode shows real
progress instead of a frozen 0% (#5); tighten the ensure_browser_playable docstring to its
actual mp4-profile scope (#6).
Mini-epic "Downloads correctness & UX" (S1–S3), on top of the iOS-AV1 + 720p fixes.
S1 — naming & titles:
- ASCII-fold on-disk dir/file names AND the served (Content-Disposition) filename: no
spaces (underscore-joined), no accents (á→a, ő→o, ß→ss), pure ASCII — portable and free
of the `?` mojibake accented names show in non-UTF-8 tools. (Forward-only; existing files
clean up via re-download.)
- Strip a leading social engagement prefix ("1.6M views · 66K reactions | …") from titles.
S2 — universal browser playability (generalizes the iOS re-encode):
- ensure_browser_playable guarantees the stored file is H.264 + AAC/MP3 in mp4 — the only
combo every browser decodes. Re-encode just the offending stream(s) (video for AV1/VP9/HEVC,
audio for Opus/etc), or remux when only the container is wrong. Gated on the mp4 compat
profile (an explicit vcodec / non-mp4 custom profile opts out).
S3 — force re-download (preserves share links):
- POST /downloads/{job_id}/redownload keeps the job row so its public DownloadLink survives
and resolves to the freshly-downloaded file; deletes the old file and re-fetches under the
current rules. A failed re-download leaves the job in error (the intended broken-link case).
- Library "Re-download" action (icon + confirm), api method, hu/en strings.
Tests: naming/title-strip (test_naming), browser_transcode_flags matrix; existing
download_filename expectation updated for the underscore policy.
Shared /watch links showed a "broken play button" on iPad (any iOS
browser — all forced onto WebKit) while playing fine on desktop Chrome.
Root cause: the stored mp4's video codec was AV1 (confirmed via ffprobe),
which WebKit can't decode. The existing H.264 guard was only a
format_sort *preference*, so a non-YouTube source (Facebook) that ships
AV1 as its best video-only stream slipped through.
- B1: for the mp4 compat profile, prefer H.264 at the SELECTION level
with a progressive fallback (bestvideo[vcodec^=avc1]+bestaudio /
best[vcodec^=avc1] / anything) so an avc1 progressive is chosen over
an AV1-only video-only stream.
- B2: re-encode the rare AV1/VP9-only result to H.264+AAC (+faststart)
in the worker before storing, gated on the compat profile; corrects
the asset codec metadata to match.
- Bump _SIG_VERSION 2->3 so cached AV1 assets re-derive.
- Default download preset is now 720p (migration 0058 re-seeds builtins
720p-first; ProfileEditor BLANK 1080->720).
- Tests for the selector, compat opt-outs, and the transcode predicate.
/code-review (S1, finding 1): sanitize truncated the title at 120 CHARACTERS, but a
path component's hard limit is 255 BYTES and rel_path packs channel + date + id + ext
into that same component — so a long accented/multi-byte title (Hungarian titles are
common here) could already overflow to ENAMETOOLONG, and the new _a{id} suffix shrank
the margin further. sanitize now truncates on a UTF-8 boundary by byte length, so the
title (≤120B) + channel (≤80B) + fixed parts stay comfortably under 255B. Two tests:
byte-cap on accented input, and every rel_path component ≤255B for a pathological
long-accented channel+title+asset_id.
A MediaAsset is unique on (source_kind, source_ref, format_sig), so the same video in
two formats is two assets — but rel_path keyed only on video_id, so they computed the
same path: the second download silently overwrote the first, and deleting either
removed the shared file. rel_path now takes an optional asset_id and appends "_a{id}"
(which Plex ignores — the .nfo sidecar carries the metadata); the worker passes the
asset id. Only affects NEW downloads (existing assets keep their stored rel_path).
Covered by a new test (two ids → two paths; omitted → the plain name, unchanged).
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.
peter