Let a queued book's cheap picks download without the rest

shelfmark/core/saved_autoget.py

@@ -75,11 +75,16 @@ class AutoGetHooks:
 
 @dataclass
 class Decision:
-    """Whether an item can be got now; if not, why it waits."""
+    """Which of an item's picks can be got now, and why the rest wait.
 
-    ready: bool
-    status: str = ""
+    `ready` holds the indexes of the picks that can go (into the item's releases and
+    payloads), with `payloads` their payloads to queue. `status` says why the others
+    wait; it is empty when none do.
+    """
+
+    ready: list[int] = field(default_factory=list)
     payloads: list[dict[str, Any]] = field(default_factory=list)
+    status: str = ""
 
 
 def enabled() -> bool:
@@ -172,51 +177,76 @@ def _unsat_wait(check: mam_account.BufferCheck) -> str:
     return f"Waiting for {needed} unsatisfied slot{'s' if needed != 1 else ''}"
 
 
+def _waiting(status: str) -> Decision:
+    return Decision(status=status)
+
+
 def evaluate(item: dict[str, Any], owner: Owner, hooks: AutoGetHooks, run: _RunContext) -> Decision:
-    """Decide whether one saved item can be downloaded now."""
+    """Decide which of a saved item's picks can be downloaded now.
+
+    Each pick is weighed on its own, so a freeleech torrent or a small ebook isn't held
+    back by a large audiobook picked with it that the ratio can't take yet: the cheapest
+    go first (nothing on MAM, then freeleech, then by size), each one that still fits
+    alongside those already going.
+    """
     releases = item.get("releases") or []
     payloads = [p for p in item.get("payloads") or [] if isinstance(p, dict)]
     if not payloads or len(payloads) != len(releases):
-        return Decision(False, "Save the pick again to download it automatically")
+        return _waiting("Save the pick again to download it automatically")
     if not all(hooks.may_download(owner, payload) for payload in payloads):
-        return Decision(False, "Needs approval: use Get to request it")
+        return _waiting("Needs approval: use Get to request it")
 
+    # Nothing counts against MyAnonamouse here, or nothing can be checked: all go.
     mam_indexes = [i for i, p in enumerate(payloads) if mam_torrent_id_for_release(p)]
-    if not mam_indexes:
-        # Nothing here counts against MyAnonamouse, so nothing to wait for.
-        return Decision(True, payloads=payloads)
-    if not mam_account.is_configured():
-        return Decision(True, payloads=payloads)
+    if not mam_indexes or not mam_account.is_configured():
+        return Decision(ready=list(range(len(payloads))), payloads=payloads)
+    free_indexes = [i for i in range(len(payloads)) if i not in mam_indexes]
 
     stats, error = run.stats()
     if stats is None:
-        return Decision(False, f"Couldn't read the MyAnonamouse account: {error}")
+        return Decision(
+            ready=free_indexes,
+            payloads=[payloads[i] for i in free_indexes],
+            status=f"Couldn't read the MyAnonamouse account: {error}",
+        )
 
     # Freeleech can start or end at any time, so ask MAM rather than trusting the save.
     vip = mam_account.is_vip_class(stats.classname)
+    checked = list(payloads)
     for index in mam_indexes:
-        torrent_id = mam_torrent_id_for_release(payloads[index])
+        torrent_id = mam_torrent_id_for_release(checked[index])
         if torrent_id is None:
             continue
         try:
             freeleech = mam_account.torrent_is_freeleech(torrent_id, vip=vip)
         except mam_account.MAM_ERRORS:
             continue  # Keep what the search said; the ratio check covers a wrong guess
-        payloads[index] = _with_freeleech(payloads[index], freeleech)
+        checked[index] = _with_freeleech(checked[index], freeleech)
+
+    going: list[int] = []
+    reason = ""
+    for index in sorted(mam_indexes, key=lambda i: mam_charge_bytes_for_release(checked[i])):
+        trial = [checked[i] for i in [*going, index]]
+        wait = _pick_wait(trial, stats)
+        if wait:
+            reason = reason or wait
+            continue
+        going.append(index)
+
+    ready = sorted([*free_indexes, *going])
+    return Decision(ready=ready, payloads=[checked[i] for i in ready], status=reason)
 
-    check = mam_account.check_buffer(payloads)
+
+def _pick_wait(trial: list[dict[str, Any]], stats: mam_account.MamStats) -> str | None:
+    """Why the last of `trial` must wait, with the rest of it going too; None if it may go."""
+    check = mam_account.check_buffer(trial)
     if not check.ok:
         if not check.unsat_ok:
-            return Decision(False, _unsat_wait(check))
-        return Decision(False, f"Waiting for buffer: {_gib(check.missing_bytes)} short")
+            return _unsat_wait(check)
+        return f"Waiting for buffer: {_gib(check.missing_bytes)} short"
     if not check.checked and check.error:
-        return Decision(False, f"Couldn't check MyAnonamouse: {check.error}")
-
-    wait = _ratio_wait(stats, sum(mam_charge_bytes_for_release(p) for p in payloads))
-    if wait:
-        return Decision(False, wait)
-
-    return Decision(True, payloads=payloads)
+        return f"Couldn't check MyAnonamouse: {check.error}"
+    return _ratio_wait(stats, sum(mam_charge_bytes_for_release(p) for p in trial))
 
 
 @dataclass
@@ -275,28 +305,35 @@ def _check_item(self, item: dict[str, Any], run: _RunContext, report: RunReport)
             self._service.update(owner_key, item["id"], auto_status=decision.status)
             return
 
-        failed: list[tuple[dict[str, Any], dict[str, Any], str]] = []
         releases = item.get("releases") or []
-        for pick, payload in zip(releases, decision.payloads, strict=True):
-            ok, error = self._hooks.queue_release(payload, owner)
+        payloads = item.get("payloads") or []
+        failed: list[int] = []
+        error = ""
+        for index, payload in zip(decision.ready, decision.payloads, strict=True):
+            ok, queue_error = self._hooks.queue_release(payload, owner)
             if not ok:
-                failed.append((pick, payload, error or "Could not queue the download"))
-        if len(failed) == len(decision.payloads):
-            error = failed[0][2]
+                failed.append(index)
+                error = error or queue_error or "Could not queue the download"
+        if len(failed) == len(decision.ready):
             report.waiting[item["id"]] = error
             self._service.update(owner_key, item["id"], auto_status=f"Couldn't queue: {error}")
             return
+
         report.queued.append(item["id"])
-        if failed:
+        left = [i for i in range(len(releases)) if i not in decision.ready or i in failed]
+        if not left:
+            self._service.delete(owner_key, item["id"])
+        else:
+            if decision.status:
+                report.waiting[item["id"]] = decision.status
             self._service.keep_picks(
                 owner_key,
                 item["id"],
-                releases=[pick for pick, _, _ in failed],
-                payloads=[payload for _, payload, _ in failed],
-                last_error=f"Some picks couldn't be queued: {failed[0][2]}",
+                releases=[releases[i] for i in left],
+                payloads=[payloads[i] for i in left],
+                last_error=f"Some picks couldn't be queued: {error}" if failed else None,
+                auto_status=decision.status,
             )
-        else:
-            self._service.delete(owner_key, item["id"])
         try:
             self._hooks.notify(item, owner)
         except Exception:  # a notification failure must not undo the download

shelfmark/core/saved_items.py

@@ -387,25 +387,32 @@ def keep_picks(
         *,
         releases: list[dict[str, Any]],
         payloads: list[dict[str, Any]],
-        last_error: str,
+        last_error: str | None = None,
+        auto_status: str | None = None,
     ) -> None:
-        """After a partly failed automatic download: keep only the picks still to get.
+        """After an automatic download got some of an item's picks: keep only the rest.
 
-        Automatic downloading is turned off so the error is seen before anything retries.
+        With `last_error` (a pick couldn't be queued), automatic downloading is turned
+        off so the error is seen before anything retries. Without it, the rest are
+        simply still waiting for room: they stay queued, with `auto_status` saying why.
         """
         kind = "release" if len(releases) == 1 else "combined"
+        failed = last_error is not None
         with self._lock:
             conn = self._connect()
             try:
                 conn.execute(
                     """UPDATE saved_items SET kind = CASE WHEN content_type = 'combined'
-                       THEN 'combined' ELSE ? END, releases = ?, payloads = ?, auto_get = 0,
-                       auto_status = NULL, last_error = ?, updated_at = ?
+                       THEN 'combined' ELSE ? END, releases = ?, payloads = ?, auto_get = ?,
+                       auto_status = ?, auto_checked_at = ?, last_error = ?, updated_at = ?
                        WHERE owner = ? AND id = ?""",
                     (
                         kind,
                         json.dumps(releases),
                         json.dumps(payloads),
+                        0 if failed else 1,
+                        None if failed else (auto_status or None),
+                        None if failed else now_utc_iso(),
                         last_error,
                         now_utc_iso(),
                         owner,

src/frontend/src/components/activity/SavedPanel.tsx

@@ -368,7 +368,7 @@ const SavedTable = ({
     <section aria-label={SAVED_STAGE_LABELS[stage]} className="space-y-2">
       <p className="text-xs opacity-60">
         {inLine
-          ? "Each downloads on its own once there's room, checked in this order: one that doesn't fit yet doesn't hold up the ones after it."
+          ? "Each release downloads on its own once there's room, checked in this order: one that doesn't fit yet (a big audiobook waiting for your ratio, say) doesn't hold up the others, even those picked with it."
           : 'Choose releases for a book and it joins the queue, to download once there’s room.'}
       </p>
       <TableFrame>

tests/core/test_saved_autoget.py

@@ -138,7 +138,7 @@ def test_gets_an_item_once_it_fits_then_removes_it(service, mam):
 
 
 def test_waits_for_unsatisfied_slots_and_says_how_many(service, mam):
-    mam.unsat_free = 1
+    mam.unsat_free = 0
     recorder = Recorder()
     item_id = _mark(service, [_payload("r1", torrent_id=1), _payload("r2", torrent_id=2)])
     report = SavedAutoGetter(service, recorder.hooks()).run_check()
@@ -151,6 +151,60 @@ def test_waits_for_unsatisfied_slots_and_says_how_many(service, mam):
     assert item["auto_get"] is True
 
 
+def test_picks_that_fit_go_and_the_rest_keep_waiting(service, mam):
+    # One slot free, two picks: the first goes now, the second waits for the next slot.
+    mam.unsat_free = 1
+    recorder = Recorder()
+    item_id = _mark(service, [_payload("r1", torrent_id=1), _payload("r2", torrent_id=2)])
+    report = SavedAutoGetter(service, recorder.hooks()).run_check()
+    assert report.queued == [item_id]
+    assert [p["source_id"] for p in recorder.queued] == ["r1"]
+    item = service.get_item("user:1", item_id)
+    assert item is not None
+    assert [pick["release"]["source_id"] for pick in item["releases"]] == ["r2"]
+    assert item["auto_get"] is True  # Still queued: nothing went wrong
+    assert item["last_error"] is None
+    assert item["auto_status"] == "Waiting for 1 unsatisfied slot"
+    assert recorder.notified == ["Book Title"]
+
+
+def test_freeleech_and_small_picks_are_not_held_back_by_a_large_one(service, mam):
+    # Ratio 1.5, under the 2.0 target. A freeleech audiobook and a small ebook can go;
+    # the 40 GB non-freeleech audiobook picked with them waits for the ratio.
+    mam.stats = _stats(uploaded=150 * GIB, downloaded=100 * GIB)
+    mam.freeleech[2] = True
+    recorder = Recorder()
+    item_id = _mark(
+        service,
+        [
+            _payload("ebook", torrent_id=1, size=5 * 1024**2),
+            _payload("fl-audiobook", torrent_id=2, size=30 * GIB),
+            _payload("big-audiobook", torrent_id=3, size=40 * GIB),
+        ],
+    )
+    report = SavedAutoGetter(service, recorder.hooks()).run_check()
+    assert report.queued == [item_id]
+    assert sorted(p["source_id"] for p in recorder.queued) == ["ebook", "fl-audiobook"]
+    item = service.get_item("user:1", item_id)
+    assert item is not None
+    assert [pick["release"]["source_id"] for pick in item["releases"]] == ["big-audiobook"]
+    assert item["auto_get"] is True
+    assert item["auto_status"].startswith("Waiting for freeleech or ratio 2.00")
+
+
+def test_the_cheapest_picks_get_the_free_slots(service, mam):
+    # One slot: the freeleech pick gets it, though it was picked after the paid one.
+    mam.unsat_free = 1
+    mam.freeleech[2] = True
+    recorder = Recorder()
+    _mark(
+        service,
+        [_payload("paid", torrent_id=1, size=GIB), _payload("freeleech", torrent_id=2, size=GIB)],
+    )
+    SavedAutoGetter(service, recorder.hooks()).run_check()
+    assert [p["source_id"] for p in recorder.queued] == ["freeleech"]
+
+
 def test_freeleech_goes_even_below_the_target_ratio(service, mam):
     # Ratio 1.5, under the 2.0 target: a freeleech torrent can't lower it, so it goes.
     mam.stats = _stats(uploaded=150 * GIB, downloaded=100 * GIB)