fix erroneous event=completed tracker announce when checking files

ChangeLog

@@ -1,5 +1,6 @@
 1.2.3 release
 
+	* fix erroneous event=completed tracker announce when checking files
 	* promote errors in parsing listen_interfaces to post listen_failed_alert
 	* fix bug in protocol encryption/obfuscation
 	* fix buffer overflow in SOCKS5 UDP logic

include/libtorrent/announce_entry.hpp

@@ -59,7 +59,7 @@ namespace libtorrent {
 #endif
 
 		// internal
-		explicit announce_endpoint(aux::listen_socket_handle const& s);
+		announce_endpoint(aux::listen_socket_handle const& s, bool completed);
 
 		// if this tracker has returned an error or warning message
 		// that message is stored here

include/libtorrent/torrent.hpp

@@ -1713,6 +1713,10 @@ namespace libtorrent {
 		// this is set to true while waiting for an async_set_file_priority
 		bool m_outstanding_file_priority:1;
 
+		// set to true if we've sent an event=completed to any tracker. This will
+		// prevent us from sending it again to anyone
+		bool m_complete_sent:1;
+
 #if TORRENT_USE_ASSERTS
 		// set to true when torrent is start()ed. It may only be started once
 		bool m_was_started = false;

simulation/setup_swarm.hpp

@@ -38,7 +38,7 @@ POSSIBILITY OF SUCH DAMAGE.
 #ifndef TORRENT_SETUP_SWARM_HPP_INCLUDED
 #define TORRENT_SETUP_SWARM_HPP_INCLUDED
 
-enum class swarm_test { download, upload };
+enum class swarm_test { download, upload, upload_no_auto_stop };
 
 void setup_swarm(int num_nodes
 	, swarm_test type

simulation/test_tracker.cpp

@@ -151,7 +151,10 @@ void test_interval(int interval)
 	}
 }
 
-TORRENT_TEST(event_completed)
+template <typename AddTorrent, typename OnAlert>
+std::vector<std::string> test_event(swarm_test const type
+	, AddTorrent add_torrent
+	, OnAlert on_alert)
 {
 	using sim::asio::ip::address_v4;
 	sim::default_config network_cfg;
@@ -162,19 +165,16 @@ TORRENT_TEST(event_completed)
 	sim::http_server http(web_server, 8080);
 
 	// the request strings of all announces
-	std::vector<std::pair<int, std::string>> announces;
+	std::vector<std::string> announces;
 
 	const int interval = 500;
-	lt::time_point start = lt::clock_type::now();
 
 	http.register_handler("/announce"
 	, [&](std::string method, std::string req
 		, std::map<std::string, std::string>&)
 	{
 		TEST_EQUAL(method, "GET");
-		int const timestamp = int(chrono::duration_cast<lt::seconds>(
-			lt::clock_type::now() - start).count());
-		announces.push_back({timestamp, req});
+		announces.push_back(req);
 
 		char response[500];
 		int const size = std::snprintf(response, sizeof(response), "d8:intervali%de5:peers0:e", interval);
@@ -184,78 +184,96 @@ TORRENT_TEST(event_completed)
 	lt::settings_pack default_settings = settings();
 	lt::add_torrent_params default_add_torrent;
 
-	int completion = -1;
-
-	setup_swarm(2, swarm_test::download, sim, default_settings, default_add_torrent
+	setup_swarm(2, type, sim, default_settings, default_add_torrent
 		// add session
 		, [](lt::settings_pack&) { }
 		// add torrent
-		, [](lt::add_torrent_params& params) {
-			params.trackers.push_back("http://2.2.2.2:8080/announce");
-		}
+		, add_torrent
 		// on alert
-		, [&](lt::alert const*, lt::session&) {}
+		, on_alert
 		// terminate
 		, [&](int const ticks, lt::session& ses) -> bool
 		{
-			if (completion == -1 && is_seed(ses))
-			{
-				completion = int(chrono::duration_cast<lt::seconds>(
-					lt::clock_type::now() - start).count());
-			}
-
 			return ticks > duration;
 		});
 
-	// the first announce should be event=started, the second should be
-	// event=completed, then all but the last should have no event and the last
-	// be event=stopped.
-	for (int i = 0; i < int(announces.size()); ++i)
-	{
-		std::string const& str = announces[i].second;
-		int timestamp = announces[i].first;
+	// this is some basic sanity checking of the announces that should always be
+	// true.
+	// the first announce should be event=started then no other announce should
+	// have event=started.
+	// only the last announce should have event=stopped.
+	TEST_CHECK(announces.size() > 2);
 
-		const bool has_start = str.find("&event=started")
-			!= std::string::npos;
-		const bool has_completed = str.find("&event=completed")
-			!= std::string::npos;
-		const bool has_stopped = str.find("&event=stopped")
-			!= std::string::npos;
+	if (announces.size() <= 2) return {};
+	TEST_CHECK(announces.front().find("&event=started") != std::string::npos);
+	for (auto const& a : span<std::string>(announces).subspan(1))
+		TEST_CHECK(a.find("&event=started") == std::string::npos);
 
-		// we there can only be one event
-		const bool has_event = str.find("&event=") != std::string::npos;
+	TEST_CHECK(announces.back().find("&event=stopped") != std::string::npos);
+	for (auto const& a : span<std::string>(announces).first(announces.size() - 1))
+		TEST_CHECK(a.find("&event=stopped") == std::string::npos);
+	return announces;
+}
 
-		std::printf("- %s\n", str.c_str());
-
-		// there is exactly 0 or 1 events.
-		TEST_EQUAL(int(has_start) + int(has_completed) + int(has_stopped)
-			, int(has_event));
-
-		switch (i)
-		{
-			case 0:
-				TEST_CHECK(has_start);
-				break;
-			case 1:
-			{
-				// the announce should have come approximately the same time we
-				// completed
-				TEST_CHECK(std::abs(completion - timestamp) <= 1);
-				TEST_CHECK(has_completed);
-				break;
-			}
-			default:
-				if (i == int(announces.size()) - 1)
-				{
-					TEST_CHECK(has_stopped);
-				}
-				else
-				{
-					TEST_CHECK(!has_event);
-				}
-				break;
+TORRENT_TEST(event_completed_downloading)
+{
+	auto const announces = test_event(swarm_test::download
+		, [](lt::add_torrent_params& params) {
+			params.trackers.push_back("http://2.2.2.2:8080/announce");
 		}
-	}
+		, [&](lt::alert const*, lt::session&) {}
+	);
+
+	// make sure there's exactly one event=completed
+	TEST_CHECK(std::count_if(announces.begin(), announces.end(), [](std::string const& s)
+		{ return s.find("&event=completed") != std::string::npos; }) == 1);
+}
+
+TORRENT_TEST(event_completed_downloading_replace_trackers)
+{
+	auto const announces = test_event(swarm_test::download
+		, [](lt::add_torrent_params& params) {}
+		, [&](lt::alert const* a, lt::session&) {
+			if (auto const* at = alert_cast<add_torrent_alert>(a))
+				at->handle.replace_trackers({announce_entry{"http://2.2.2.2:8080/announce"}});
+		}
+	);
+
+	// make sure there's exactly one event=completed
+	TEST_CHECK(std::count_if(announces.begin(), announces.end(), [](std::string const& s)
+		{ return s.find("&event=completed") != std::string::npos; }) == 1);
+}
+
+TORRENT_TEST(event_completed_seeding)
+{
+	auto const announces = test_event(swarm_test::upload_no_auto_stop
+		, [](lt::add_torrent_params& params) {
+			params.trackers.push_back("http://2.2.2.2:8080/announce");
+		}
+		, [&](lt::alert const*, lt::session&) {}
+		);
+
+	// make sure there are no event=completed, since we added the torrent as a
+	// seed
+	TEST_CHECK(std::count_if(announces.begin(), announces.end(), [](std::string const& s)
+		{ return s.find("&event=completed") != std::string::npos; }) == 0);
+}
+
+
+TORRENT_TEST(event_completed_seeding_replace_trackers)
+{
+	auto const announces = test_event(swarm_test::upload_no_auto_stop
+		, [](lt::add_torrent_params& params) {}
+		, [&](lt::alert const* a, lt::session&) {
+			if (auto const* at = alert_cast<add_torrent_alert>(a))
+				at->handle.replace_trackers({announce_entry{"http://2.2.2.2:8080/announce"}});
+		}
+	);
+
+	// make sure there are no event=completed, since we added the torrent as a
+	// seed
+	TEST_CHECK(std::count_if(announces.begin(), announces.end(), [](std::string const& s)
+		{ return s.find("&event=completed") != std::string::npos; }) == 0);
 }
 
 TORRENT_TEST(announce_interval_440)

src/announce_entry.cpp

@@ -47,13 +47,13 @@ namespace libtorrent {
 		minutes32 constexpr tracker_retry_delay_max{60};
 	}
 
-	announce_endpoint::announce_endpoint(aux::listen_socket_handle const& s)
+	announce_endpoint::announce_endpoint(aux::listen_socket_handle const& s, bool const completed)
 		: local_endpoint(s ? s.get_local_endpoint() : tcp::endpoint())
 		, socket(s)
 		, fails(0)
 		, updating(false)
 		, start_sent(false)
-		, complete_sent(false)
+		, complete_sent(completed)
 		, triggered_manually(false)
 	{}
 

src/torrent.cpp

@@ -228,6 +228,7 @@ bool is_downloading_state(int const st)
 		, m_progress_ppm(0)
 		, m_torrent_initialized(false)
 		, m_outstanding_file_priority(false)
+		, m_complete_sent(false)
 	{
 		// we cannot log in the constructor, because it relies on shared_from_this
 		// being initialized, which happens after the constructor returns.
@@ -2925,7 +2926,7 @@ bool is_downloading_state(int const st)
 					return;
 				}
 
-				ae.endpoints.emplace_back(s);
+				ae.endpoints.emplace_back(s, bool(m_complete_sent));
 				std::swap(ae.endpoints[valid_endpoints], ae.endpoints.back());
 				valid_endpoints++;
 			});
@@ -2986,7 +2987,12 @@ bool is_downloading_state(int const st)
 				if (req.event == tracker_request::none)
 				{
 					if (!aep.start_sent) req.event = tracker_request::started;
-					else if (!aep.complete_sent && is_seed()) req.event = tracker_request::completed;
+					else if (!m_complete_sent
+						&& !aep.complete_sent
+						&& is_seed())
+					{
+						req.event = tracker_request::completed;
+					}
 				}
 
 				req.triggered_manually = aep.triggered_manually;
@@ -3218,6 +3224,10 @@ bool is_downloading_state(int const st)
 		tcp::endpoint local_endpoint;
 		if (ae)
 		{
+#if TORRENT_ABI_VERSION == 1
+			if (!ae->complete_sent && r.event == tracker_request::completed)
+				ae->complete_sent = true;
+#endif
 			announce_endpoint* aep = ae->find_endpoint(r.outgoing_socket);
 			if (aep)
 			{
@@ -3228,7 +3238,13 @@ bool is_downloading_state(int const st)
 				if (!aep->start_sent && r.event == tracker_request::started)
 					aep->start_sent = true;
 				if (!aep->complete_sent && r.event == tracker_request::completed)
+				{
 					aep->complete_sent = true;
+					// we successfully reported event=completed to one tracker. Don't
+					// send it to any other ones from now on (there may be other
+					// announces outstanding right now though)
+					m_complete_sent = true;
+				}
 				ae->verified = true;
 				aep->next_announce = now + interval;
 				aep->min_announce = now + resp.min_interval;
@@ -5246,8 +5262,11 @@ bool is_downloading_state(int const st)
 		{
 			t.endpoints.clear();
 			if (t.source == 0) t.source = announce_entry::source_client;
+#if TORRENT_ABI_VERSION == 1
+			t.complete_sent = m_complete_sent;
+#endif
 			for (auto& aep : t.endpoints)
-				aep.complete_sent = is_seed();
+				aep.complete_sent = m_complete_sent;
 		}
 
 		if (settings().get_bool(settings_pack::prefer_udp_trackers))
@@ -7554,9 +7573,18 @@ bool is_downloading_state(int const st)
 		}
 		else
 		{
+			// we just added this torrent as a seed, or force-rechecked it, and we
+			// have all of it. Assume that we sent the event=completed when we
+			// finished downloading it, and don't send any more.
+			m_complete_sent = true;
 			for (auto& t : m_trackers)
+			{
+#if TORRENT_ABI_VERSION == 1
+				t.complete_sent = true;
+#endif
 				for (auto& aep : t.endpoints)
 					aep.complete_sent = true;
+			}
 
 			if (m_state != torrent_status::finished
 				&& m_state != torrent_status::seeding)

test/test_primitives.cpp

@@ -47,7 +47,7 @@ TORRENT_TEST(retry_interval)
 	// make sure the retry interval keeps growing
 	// on failing announces
 	announce_entry ae("dummy");
-	ae.endpoints.emplace_back(aux::listen_socket_handle());
+	ae.endpoints.emplace_back(aux::listen_socket_handle(), false);
 	int last = 0;
 	auto const tracker_backoff = 250;
 	for (int i = 0; i < 10; ++i)