From 857e51313499caceb0ad16663170cefe69f136a7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= <philipp.krueger1@gmail.com>
Date: Thu, 26 Sep 2024 17:25:31 +0200
Subject: [PATCH] fix(iroh-blobs): Remove debugging logs & more cleanup (#2690)

## Description

More stuff to clean up I came across while trying to debug an issue.
Also, this makes it so that errors from `iroh_blobs::get::db::get_blob`
that happen *before* the `get_conn` step get printed.
<!-- A summary of what this pull request achieves and a rough list of
changes. -->

## Breaking Changes

None.
<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->

## Notes & open questions

<!-- Any notes, remarks or open questions you have to make about the PR.
-->
I decided to just remove the tracing logs in `downloader/progress.rs`.
They never really helped us surface the bug, they mostly polluted the
logs, to be honest.
The only thing they surfaced was that we didn't properly clean up
`TransferState::progress_id_to_blob` when there's an error in
`iroh_blobs::get::db::get_blob` that causes us to not send the
`DownloadProgress::Done` event.

## Change checklist

- [x] Self-review.
- ~~[ ] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.~~
- ~~[ ] Tests if relevant.~~
- [x] All breaking changes documented.
---
 iroh-blobs/src/downloader.rs          | 5 +++--
 iroh-blobs/src/downloader/progress.rs | 3 ---
 iroh-blobs/src/get/db.rs              | 2 +-
 iroh-blobs/src/get/progress.rs        | 2 +-
 4 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/iroh-blobs/src/downloader.rs b/iroh-blobs/src/downloader.rs
index 2998f81ba7..ee8952550d 100644
--- a/iroh-blobs/src/downloader.rs
+++ b/iroh-blobs/src/downloader.rs
@@ -717,7 +717,6 @@ impl<G: Getter<Connection = D::Connection>, D: Dialer> Service<G, D> {
                 entry.get_mut().intents.insert(intent_id, intent_handlers);
             }
             hash_map::Entry::Vacant(entry) => {
-                tracing::warn!("is new, queue");
                 let progress_sender = self.progress_tracker.track(
                     kind,
                     intent_handlers
@@ -728,7 +727,9 @@ impl<G: Getter<Connection = D::Connection>, D: Dialer> Service<G, D> {
                 );
 
                 let get_state = match self.getter.get(kind, progress_sender.clone()).await {
-                    Err(_err) => {
+                    Err(err) => {
+                        // This prints a "FailureAction" which is somewhat weird, but that's all we get here.
+                        tracing::error!(?err, "failed queuing new download");
                         self.finalize_download(
                             kind,
                             [(intent_id, intent_handlers)].into(),
diff --git a/iroh-blobs/src/downloader/progress.rs b/iroh-blobs/src/downloader/progress.rs
index 33a46ecde9..eac80985d5 100644
--- a/iroh-blobs/src/downloader/progress.rs
+++ b/iroh-blobs/src/downloader/progress.rs
@@ -103,7 +103,6 @@ struct Inner {
 
 impl Inner {
     fn subscribe(&mut self, subscriber: ProgressSubscriber) -> DownloadProgress {
-        tracing::warn!(state=?self.state, "subscribe! emit initial");
         let msg = DownloadProgress::InitialState(self.state.clone());
         self.subscribers.push(subscriber);
         msg
@@ -137,9 +136,7 @@ impl ProgressSender for BroadcastProgressSender {
         // making sure that the lock is not held across an await point.
         let futs = {
             let mut inner = self.shared.lock();
-            tracing::trace!(?msg, state_pre = ?inner.state, "send to {}", inner.subscribers.len());
             inner.on_progress(msg.clone());
-            tracing::trace!(state_post = ?inner.state, "send");
             let futs = inner
                 .subscribers
                 .iter_mut()
diff --git a/iroh-blobs/src/get/db.rs b/iroh-blobs/src/get/db.rs
index 3aef19e998..afce797772 100644
--- a/iroh-blobs/src/get/db.rs
+++ b/iroh-blobs/src/get/db.rs
@@ -663,7 +663,7 @@ pub enum DownloadProgress {
         /// The offset of the progress, in bytes.
         offset: u64,
     },
-    /// We are done with `id`, and the hash is `hash`.
+    /// We are done with `id`.
     Done {
         /// The unique id of the entry.
         id: u64,
diff --git a/iroh-blobs/src/get/progress.rs b/iroh-blobs/src/get/progress.rs
index 5fcac04b2f..d6a900195a 100644
--- a/iroh-blobs/src/get/progress.rs
+++ b/iroh-blobs/src/get/progress.rs
@@ -180,7 +180,7 @@ impl TransferState {
                     warn!(%id, "Received `Done` event for unknown progress id.")
                 }
             }
-            _ => {}
+            DownloadProgress::AllDone(_) | DownloadProgress::Abort(_) => {}
         }
     }
 }