Overhaul stats: fix bug in downloads counter#1503
Merged
josecelano merged 1 commit intoMay 7, 2025
Merged
Conversation
… metric Relates to: torrust@34c159a A couple of days ago, I made a change in [this commit](torrust@34c159a). I changed the `Swarm::meets_retaining_policy` method from: ``` /// Returns true if the torrents meets the retention policy, meaning that /// it should be kept in the tracker. pub fn meets_retaining_policy(&self, policy: &TrackerPolicy) -> bool { if policy.persistent_torrent_completed_stat && self.metadata().downloaded > 0 { return true; } if policy.remove_peerless_torrents && self.is_empty() { return false; } true } ``` To: ``` pub fn meets_retaining_policy(&self, policy: &TrackerPolicy) -> bool { !(policy.remove_peerless_torrents && self.is_empty()) } ``` I thought this code was not needed: ```rust if policy.persistent_torrent_completed_stat && self.metadata().downloaded > 0 { return true; } ``` However, it's needed. One of the metrics returned by the tracker API is the **total number of downloads for all torrents**. ```json { "torrents": 320961, "seeders": 189885, "completed": 975119, <- this "leechers": 231044, ... } ``` That metric is always stored in memory but can optionally persist into the database. It's important to highlight that the metric represents: - The total number of downloads for **ALL** torrents ever, when the metric is persisted. - The total number of downloads for **ALL** torrents since the tracker started, when the metric is not persisted. It could be mixed up with another internal metric (not exposed via the API), which is the same counter but only for ONE swarm (one torrent). - The total number of downloads for **ONE** concrete torrent ever, when the metric is persisted. - The total number of downloads for **ONE** concrete torrent since the tracker started, when the metric is not persisted. The bug affects the first metric. The exposed via the API. The problem is that this feature conflicts with removing the peerless torrents. When removing the peerless torrents config option is enabled, the counter is lost unless it is persisted. Becuase the counter values are stored in the "Swarm" together with the list of peers. If statistics persistence is enabled, that's not a problem. When the torrent is removed from the tracker (from the swarms or swarm collection), the counter is initialised again if the torrent is added. In other words, if a new peer starts the swarm again, the number of downloads is loaded from the database. However, that works for the counter of each torrent (swarm) but not for the overall counter (the sum of downloads for all torrents). That metric is not stored anywhere. It's calculated on demand by iterating all the swarms and summing up the total for each torrent, giving the total amount of downloads for **ALL** torrents. When the torrent is removed, the downloads for that torrent don't count in the total. That is the reason we have to keep the torrent (swarm) in memory, even if it does not have any peer (and it should be removed according to the other config flag). The removed line: ```rust if policy.persistent_torrent_completed_stat && self.metadata().downloaded > 0 { return true; } ``` does that. **When the stats persistence is disabled**, that's one way to store the value. Alternatively, we could add another cache for the data and never remove that value. The current solution has a problem: It can make the tracker consume a lot of memory because peerless torrents are not removed in practice (even if it's configured to be). **When the stats persistence is enabled,** we can simply return the value from the database. **NOTICE:** that the value is used in the scrape response, so it might be convenient to have a cache in memory anyway. - [x] Revert the change to fix the bug asap. - [x] Write a unit test. This behaviour was not covered by any test (or documented). - [ ] Add an in-memory cache value in `Swarms` type to store the total for all torrents, regardless of which are the current active swarms.
Member
Author
|
ACK 32a37d1 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1503 +/- ##
===========================================
+ Coverage 83.81% 83.84% +0.03%
===========================================
Files 257 257
Lines 17837 17876 +39
Branches 17837 17876 +39
===========================================
+ Hits 14950 14989 +39
Misses 2616 2616
Partials 271 271 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates to: #1502
Revert the change that introduced a bug in this commit.
Issue #1502 has been partially implemented.
In a future PR, I will implement a change to avoid keeping the swarms in memory just to track the number of downloads for that torrent.