Skip to content

Commit f073df8

Browse files
committed
feat(downloader): NF26 — yt-dlp auto-update active-download guard
maybe_auto_update_ytdlp() used to fire fire-and-forget; yt-dlp -U atomically replaces the binary, and on Windows an in-flight subprocess.Popen([YTDLP_PATH, ...]) could race the replace with file-in-use errors. New optional active_count_fn parameter — when supplied and returns > 0, the update is deferred and the next 24h throttle window picks it up. Caller in the GUI server-start path now passes self.dl_manager.active_count so the check consults the live queue without coupling the function to the manager instance. Probe failures (raising callable) fall through to 'proceed with warning' — failure mode of an under-construction probe is at least as bad as racing the self-replace. Back-compat preserved: calling maybe_auto_update_ytdlp(config) without the new arg still works exactly as before. Touches: astra_downloader/astra_downloader.py — signature + guard at the top of the function (lines 981-1020 region); call site at the GUI server-start hook (line 3758) now passes the active_count callable astra_downloader/test_astra_downloader.py — new AutoUpdateActiveDownloadGuardTests class with four cases: fires_when_zero / defers_when_positive / proceeds_when_probe_raises / back_compat_no_arg Closes NF26 from RESEARCH_FEATURE_PLAN. Verified: 92/92 Python tests pass (+4 new).
1 parent 52664a8 commit f073df8

4 files changed

Lines changed: 178 additions & 7 deletions

File tree

CHANGELOG.md

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,25 @@ All notable changes to Astra Deck are documented here. Versions are listed newes
66

77
## [Unreleased]
88

9+
- **astra_downloader: yt-dlp auto-update active-download guard (NF26).**
10+
`maybe_auto_update_ytdlp()` (astra_downloader.py:981) used to fire
11+
fire-and-forget; `yt-dlp.exe -U` atomically replaces the binary, and
12+
on Windows an in-flight `subprocess.Popen([YTDLP_PATH, ...])` could
13+
race the replace with file-in-use errors. New optional
14+
`active_count_fn` parameter — when supplied and returns > 0, the
15+
update is deferred and the next 24h throttle window picks it up.
16+
Caller in the GUI server-start path now passes
17+
`self.dl_manager.active_count` so the check consults the live queue
18+
without coupling the function to the manager instance. Probe
19+
failures (raising callable) fall through to "proceed with warning"
20+
— failure mode of an under-construction probe is at least as bad
21+
as racing the self-replace. Back-compat preserved: calling
22+
`maybe_auto_update_ytdlp(config)` without the new arg still works
23+
exactly as before. Pinned by four new `AutoUpdateActiveDownloadGuardTests`
24+
in `test_astra_downloader.py`:
25+
fires-when-zero / defers-when-positive / proceeds-when-probe-raises
26+
/ back-compat-no-arg. 92/92 Python tests pass (+4 new).
27+
928
- **check-versions: SETTINGS_VERSION parity gate (NF25).** The product
1029
version is enforced across 5 sources by `scripts/check-versions.js`;
1130
the schema-version namespace was not. Drift caught: `popup.js`

RESEARCH_FEATURE_PLAN.md

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,11 +37,6 @@ Each item carries: priority, complexity, why, evidence, touches, acceptance, ver
3737

3838
### Settings / downloader hardening (P0 batch from 2026-05-25 audit)
3939

40-
- **P0 / M — yt-dlp auto-update active-download guard (NF26)**
41-
- Why: `astra_downloader.py:981-1018` `maybe_auto_update_ytdlp()` fires fire-and-forget at `:3758` with no `active_count() == 0` guard. Windows file-locking can fail an in-flight download mid-`-U`.
42-
- Touches: `astra_downloader/astra_downloader.py`, `astra_downloader/test_astra_downloader.py`.
43-
- Acceptance: pytest with `active_count` mock returning >0 → update deferred; ==0 → update fires.
44-
4540
- **P0 / M — Deno cutoff hard-gate on `/download` (NF27)**
4641
- Why: yt-dlp ≥ 2026.04.01 requires Deno. The `denoRuntime` probe at `astra_downloader.py:751-804` reports state but `/download` doesn't consult it; yt-dlp returns empty format lists with an opaque error.
4742
- Touches: `astra_downloader.py` (`/download` handler), `extension/ytkit.js` MediaDLManager error path.

astra_downloader/astra_downloader.py

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -978,18 +978,48 @@ def mark_ytdlp_update_check(config):
978978
write_persistent_log(f"Could not persist yt-dlp update timestamp: {e}")
979979

980980

981-
def maybe_auto_update_ytdlp(config):
981+
def maybe_auto_update_ytdlp(config, active_count_fn=None):
982982
"""Background-run yt-dlp -U when more than a day has passed.
983983
984984
Fire-and-forget in a daemon thread so startup isn't blocked. The exit code
985985
is logged (previously swallowed entirely).
986+
987+
v4.47.0 NF26: when ``active_count_fn`` is provided and returns > 0, the
988+
update is deferred. yt-dlp's ``-U`` flag atomically replaces the binary,
989+
and on Windows the in-flight ``subprocess.Popen([YTDLP_PATH, ...])`` of
990+
an active download can race the replace with file-in-use errors. Callers
991+
in the GUI / server path pass ``dl_manager.active_count`` so the check
992+
consults the live queue without coupling this function to the manager
993+
instance.
994+
995+
The check is racy by design — we accept that a download started during
996+
the millisecond between ``active_count_fn()`` returning 0 and ``-U``
997+
spawning the replacement process can still race. Mitigation in practice
998+
is that ``-U`` runs ahead of any user download (server-start hook), and
999+
a download starting in that micro-window is so unlikely we don't pay the
1000+
cost of a hard cross-process lock.
9861001
"""
9871002
if not YTDLP_PATH.exists():
9881003
return
9891004
if not config.get("AutoUpdateYtDlp", True):
9901005
return
9911006
if not should_check_ytdlp_update(config):
9921007
return
1008+
if active_count_fn is not None:
1009+
try:
1010+
in_flight = int(active_count_fn() or 0)
1011+
except Exception as e: # noqa: BLE001
1012+
# reason: active_count_fn is caller-supplied; if it raises we
1013+
# must NOT block the update, since the caller's failure mode
1014+
# is at least as bad as racing a self-replace.
1015+
write_persistent_log(f"yt-dlp auto-update active-count probe failed: {e}")
1016+
in_flight = 0
1017+
if in_flight > 0:
1018+
write_persistent_log(
1019+
f"yt-dlp auto-update deferred — {in_flight} active download(s); "
1020+
f"next check at the configured 24h throttle."
1021+
)
1022+
return
9931023

9941024
def run():
9951025
try:
@@ -3755,7 +3785,10 @@ def run():
37553785
# Auto-update yt-dlp — throttled (once per 24h) so we don't re-run
37563786
# it on every single launch. Logs exit code instead of silently
37573787
# discarding it.
3758-
maybe_auto_update_ytdlp(self.config)
3788+
#
3789+
# v4.47.0 NF26: pass the manager's active_count so an in-flight
3790+
# download isn't raced by a yt-dlp.exe self-replace.
3791+
maybe_auto_update_ytdlp(self.config, self.dl_manager.active_count)
37593792

37603793
def _stop_server(self):
37613794
if self.server_obj:

astra_downloader/test_astra_downloader.py

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -693,6 +693,130 @@ def get(self, key, default=None):
693693
self.assertTrue(ad.should_check_ytdlp_update(_C()))
694694

695695

696+
class AutoUpdateActiveDownloadGuardTests(unittest.TestCase):
697+
"""v4.47.0 NF26 — yt-dlp's ``-U`` flag atomically replaces the binary.
698+
On Windows an in-flight ``subprocess.Popen([YTDLP_PATH, ...])`` of an
699+
active download can race the replace with file-in-use errors. The
700+
guard takes a caller-supplied ``active_count_fn`` and defers the
701+
update when the function reports any in-flight downloads.
702+
"""
703+
704+
def _fake_config(self, last_check_stamp=""):
705+
# Empty stamp triggers should_check_ytdlp_update -> True.
706+
class _C:
707+
def __init__(self, stamp):
708+
self._d = {
709+
"AutoUpdateYtDlp": True,
710+
"LastYtDlpUpdateCheck": stamp,
711+
}
712+
def get(self, key, default=None):
713+
return self._d.get(key, default)
714+
def set(self, key, value):
715+
self._d[key] = value
716+
def save(self):
717+
pass
718+
return _C(last_check_stamp)
719+
720+
def test_update_fires_when_no_active_downloads(self):
721+
# active_count_fn returning 0 must NOT defer the update; the
722+
# threading.Thread must be spawned (we mock it to track the spawn
723+
# without actually running yt-dlp).
724+
spawned = {"count": 0}
725+
orig_thread = ad.threading.Thread
726+
try:
727+
class _FakeThread:
728+
def __init__(self, target=None, daemon=None):
729+
self._target = target
730+
def start(self):
731+
spawned["count"] += 1
732+
# Don't actually run -U; this tests the guard, not the
733+
# subprocess invocation.
734+
ad.threading.Thread = _FakeThread
735+
# YTDLP_PATH must exist for the guard to fall through to the
736+
# active-count check; patch its existence check.
737+
ad.YTDLP_PATH = type(ad.YTDLP_PATH)(ad.YTDLP_PATH)
738+
with mock.patch.object(ad.YTDLP_PATH.__class__, 'exists', return_value=True):
739+
ad.maybe_auto_update_ytdlp(self._fake_config(), active_count_fn=lambda: 0)
740+
finally:
741+
ad.threading.Thread = orig_thread
742+
self.assertEqual(spawned["count"], 1,
743+
"Update thread must spawn when active_count == 0")
744+
745+
def test_update_defers_when_active_downloads_in_flight(self):
746+
# active_count_fn returning > 0 must defer the update; no thread
747+
# should spawn.
748+
spawned = {"count": 0}
749+
log_lines = []
750+
orig_thread = ad.threading.Thread
751+
orig_log = ad.write_persistent_log
752+
try:
753+
class _FakeThread:
754+
def __init__(self, target=None, daemon=None):
755+
pass
756+
def start(self):
757+
spawned["count"] += 1
758+
ad.threading.Thread = _FakeThread
759+
ad.write_persistent_log = lambda msg: log_lines.append(msg)
760+
with mock.patch.object(ad.YTDLP_PATH.__class__, 'exists', return_value=True):
761+
ad.maybe_auto_update_ytdlp(self._fake_config(), active_count_fn=lambda: 3)
762+
finally:
763+
ad.threading.Thread = orig_thread
764+
ad.write_persistent_log = orig_log
765+
self.assertEqual(spawned["count"], 0,
766+
"Update thread must NOT spawn when active_count > 0")
767+
self.assertTrue(any("auto-update deferred" in line for line in log_lines),
768+
f"Defer log line must surface in persistent log; got {log_lines!r}")
769+
self.assertTrue(any("3 active download" in line for line in log_lines),
770+
f"Defer log must include the count; got {log_lines!r}")
771+
772+
def test_update_proceeds_when_active_count_fn_raises(self):
773+
# Caller-supplied probe failure must not block the update.
774+
# Failure mode of an under-construction probe is at least as bad
775+
# as racing a self-replace, so we prefer "proceed with warning".
776+
spawned = {"count": 0}
777+
log_lines = []
778+
orig_thread = ad.threading.Thread
779+
orig_log = ad.write_persistent_log
780+
try:
781+
class _FakeThread:
782+
def __init__(self, target=None, daemon=None):
783+
pass
784+
def start(self):
785+
spawned["count"] += 1
786+
ad.threading.Thread = _FakeThread
787+
ad.write_persistent_log = lambda msg: log_lines.append(msg)
788+
789+
def broken_probe():
790+
raise RuntimeError("probe boom")
791+
with mock.patch.object(ad.YTDLP_PATH.__class__, 'exists', return_value=True):
792+
ad.maybe_auto_update_ytdlp(self._fake_config(), active_count_fn=broken_probe)
793+
finally:
794+
ad.threading.Thread = orig_thread
795+
ad.write_persistent_log = orig_log
796+
self.assertEqual(spawned["count"], 1,
797+
"Update thread must still spawn when probe raises")
798+
self.assertTrue(any("probe failed" in line for line in log_lines),
799+
f"Probe-failure log line must surface; got {log_lines!r}")
800+
801+
def test_update_without_active_count_fn_proceeds(self):
802+
# Back-compat: existing callers without the new arg must still work.
803+
spawned = {"count": 0}
804+
orig_thread = ad.threading.Thread
805+
try:
806+
class _FakeThread:
807+
def __init__(self, target=None, daemon=None):
808+
pass
809+
def start(self):
810+
spawned["count"] += 1
811+
ad.threading.Thread = _FakeThread
812+
with mock.patch.object(ad.YTDLP_PATH.__class__, 'exists', return_value=True):
813+
ad.maybe_auto_update_ytdlp(self._fake_config())
814+
finally:
815+
ad.threading.Thread = orig_thread
816+
self.assertEqual(spawned["count"], 1,
817+
"Update must still fire when no active_count_fn provided")
818+
819+
696820
class NoArchiveLockTests(unittest.TestCase):
697821
"""v1.3.0 removed the download-archive lock so re-downloads always
698822
run. These tests pin the invariants so the lock can't be silently

0 commit comments

Comments
 (0)