Commit Graph
8 Commits
Author SHA1 Message Date
peter 86bc008a81 fix: a crashed job's crash line carries its job id
The bind sat inside the try, so it was already reset when the crash line and the cleanup were logged — the one line most worth finding by job id had none.
2026-09-17 03:53:29 +02:00
peter e887233210 fix(downloads): E11.5 epic-end review round 1
A refused Facebook cookie on a group post is no longer "no video in this post":
measured on dev, Facebook answers with its login page (no post id at all), so the
adapter now raises its own POST_NOT_SHOWN verdict and the catalog explains it as
the refused cookie. The adapter owns both post verdicts and the "cannot parse
data" wall wording, so the Chrome retry and the catalog read one constant. The
group-post "no formats" entry is cookie-scoped (with a cookie yt-dlp only gets the
post's video link) and uses the adapter's own group-post test instead of a
"/groups/" substring.

Job transitions that move an asset's ref count (resume, cancel, delete,
redownload) re-read the job under a row lock, asset first, so two overlapping
requests can no longer release the same hold. get_or_create_asset loses an insert
race inside a SAVEPOINT instead of rolling back the caller's pending writes (Retry
lost its cleared error). Every worker-side job failure goes through
finalize_if_running and writes error_detail, so a cancel that landed first is kept
and a failure never shows an earlier one's copy. The worker asks the download
child's own cookie gate (credentials.usable_cookie) when a job inherits a failed
asset.

UI: the Facebook copy no longer points at a Retry button the failure window does
not have, says "paste" like Settings, and the HU wording is fixed
("Újraletöltés" on the re-download confirm). A category this client has no copy
for falls back to the generic sentence instead of a raw key.

Tests: route serialization of error_detail, the probed duration through a real
_download, the page-read cap and a failed read, a Retry off a pending asset, the
confirm-button labels, the lock and savepoint races.
2026-09-14 00:50:05 +02:00
peter b1832afd18 style: apply ruff format across backend + scripts (line-length 100)
Mechanical, whole-tree `ruff format` pass — no behaviour change. Recorded in
.git-blame-ignore-revs (next commit) so git blame skips it.
2026-08-23 07:43:06 +02:00
peter 367bcebc9f fix(downloads): revive the worker cancel branch and guard terminal writes (B-sub-1)
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.)
2026-08-18 00:21:43 +02:00
peter cce862cde7 fix(downloads): supervisor edge-cases from review round 3
- worker.py: keep an absolute wall-clock backstop for ANY still-alive child. A child that
  reached a terminal state (done/error) but wedges in teardown is now killed to free the worker
  slot ("abandon"), with its DB status left intact; only a still-'running' over-cap job is failed.
  Previously gating the wall-clock on status=='running' let a wedged finisher loop forever (#1).
- worker.py: reconcile the claimed asset on ANY clean child exit (not only non-zero), so a child
  that exits without finalizing it — e.g. an early return on a job DELETEd mid-flight — no longer
  leaves the asset stuck 'downloading' until boot recovery (#2).
- worker.py: extract _stop_and_free + _fail_after_kill; the exception / stall / timeout / cancel /
  shutdown / abandon paths now share one kill+free (+fail) tail instead of three copies (#3).
- tests: a wedged-finished child is killed by the wall-clock without a status change; clean exits
  now assert the reconciliation no-op (#1, #4).
2026-08-12 23:38:37 +02:00
peter b42b340594 fix(downloads): don't let the supervisor kill a finishing child (review round 2)
- worker.py: the watchdog loop now treats only 'paused'/'cancelled' (or a vanished row) as a
  stop; a terminal status (done/error, or a re-queued contended asset) means the child already
  wrote its outcome and is exiting, so keep polling instead of killing it. Previously a
  proc.wait timeout landing between the child's terminal commit and its exit was misread as a
  route-cancel and SIGKILLed a successful download (dropping its completion notification). The
  stall/wall-clock/shutdown checks are now gated on status=='running' (#1, #5 test).
- worker.py: guard the crash-path _job_liveness probe so a DB blip there can't propagate (the
  child is already dead; boot recovery is the backstop) (#2).
- worker.py: drop _stop_child's redundant poll guard — escalate_kill no-ops on an exited proc
  and getpgid yields None for a reaped pid (#3).
- worker.py: note the extraction heartbeat is per-attempt (a single over-timeout attempt is
  still killed, bounded by socket_timeout) (#4).
2026-08-12 23:15:41 +02:00
peter 657224c1ad refactor(downloads): address /code-review on the download supervisor
- 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).
2026-08-12 23:04:48 +02:00
peter 0ed82fe6b6 feat(downloads): run each download/edit in a supervised child process with a hard-kill watchdog
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).
2026-08-12 22:39:17 +02:00