diff --git a/src/common/http/http_resumer.cpp b/src/common/http/http_resumer.cpp index 172d6abd8..699fe8f87 100644 --- a/src/common/http/http_resumer.cpp +++ b/src/common/http/http_resumer.cpp @@ -472,6 +472,23 @@ error::Error DownloadResumerClient::ScheduleNextResumeRequest() { void DownloadResumerClient::CallUserHandler(http::ExpectedIncomingResponsePtr exp_resp) { if (!exp_resp) { DoCancel(); + + // If a read is pending on the body reader, complete it with the error as well. When the + // body is consumed through a DownloadResumerAsyncReader (streaming download, as the + // update state machine does), a terminal error such as giving up on resuming after the + // retry backoff is exhausted only reaches the user through the user body handler below. + // But that handler cannot recover a streaming read, and the update client intentionally + // ignores errors there, relying on them surfacing through the reader instead. Without + // completing the pending read, that consumer blocks forever and the daemon wedges + // without ever reporting a deployment failure. Delivering the error here does not + // disturb the buffered SetBodyWriter case, where the pending read belongs to an internal + // AsyncCopy that merely logs the error, and the user body handler below still runs. + auto resumer_reader = resumer_reader_.lock(); + if (resumer_reader && last_read_.handler) { + auto handler = last_read_.handler; + last_read_.handler = nullptr; + handler(expected::unexpected(exp_resp.error())); + } } if (resumer_state_->user_handlers_state == DownloadResumerUserHandlersStatus::None) { resumer_state_->user_handlers_state = diff --git a/tests/src/common/http_resumer/http_resumer_test.cpp b/tests/src/common/http_resumer/http_resumer_test.cpp index 1f7fb8099..b7fa4485e 100644 --- a/tests/src/common/http_resumer/http_resumer_test.cpp +++ b/tests/src/common/http_resumer/http_resumer_test.cpp @@ -692,6 +692,114 @@ TEST_F(DownloadResumerTest, ResponseBodyReaderSmallBuffer) { EXPECT_FALSE(got_read_error); } +// Regression test for the daemon hang reported in +// https://hub.mender.io/t/.../8299 : when the body is consumed through the async reader +// (as the update state machine does) and the server stays down until the resumer exhausts +// its retry backoff, the "giving up" error must be delivered through the reader's pending +// read handler. Before the fix it was only routed to the user body handler (which the update +// client intentionally ignores), so the pending read never completed and the daemon wedged +// forever without reporting a deployment failure. +TEST_F(DownloadResumerTest, ServerDownWhileReadingBodyReportsError) { + TestEventLoop loop(chrono::seconds(30)); + + http::ServerConfig server_config; + http::Server server(server_config, loop); + events::Timer kill_timer(loop); + bool kill_scheduled {false}; + + int server_num_requests = 0; + server.AsyncServeUrl( + "http://127.0.0.1:" TEST_PORT, + [](http::ExpectedIncomingRequestPtr exp_req) { + ASSERT_TRUE(exp_req) << exp_req.error().String(); + }, + [&server, &server_num_requests, &kill_timer, &kill_scheduled]( + http::ExpectedIncomingRequestPtr exp_req) { + ASSERT_TRUE(exp_req) << exp_req.error().String(); + + auto result = exp_req.value()->MakeResponse(); + ASSERT_TRUE(result); + auto resp = result.value(); + + server_num_requests++; + + auto size = RangeBodyOfXes::TARGET_BODY_SIZE; + resp->SetStatusCodeAndMessage(200, "Success"); + // Announce the full length, but only deliver a small part of it, so the client + // sees a short read and tries to resume. Keep it small so the first response + // finishes well before the server is taken down below. + resp->SetHeader("Content-Length", to_string(size)); + auto partial_body = make_shared(); + partial_body->SetRanges(0, 4999); + resp->SetBodyReader(partial_body); + + resp->AsyncReply([](error::Error err) { ASSERT_EQ(error::NoError, err); }); + + // After the first (partial) response, take the server down and keep it down, so + // every resume attempt fails until the resumer gives up. Cancelling from a timer + // (rather than the reply handler) avoids tearing down the stream mid-write. + if (!kill_scheduled) { + kill_scheduled = true; + kill_timer.AsyncWait( + chrono::milliseconds(100), [&server](error::Error) { server.Cancel(); }); + } + }); + + http::ClientConfig client_config; + shared_ptr client = + make_shared(client_config, loop); + // First resume waits longer than the server-kill delay above, so the server is already + // down by the time we retry, and the retries are then exhausted quickly. + client->SetSmallestWaitInterval(chrono::milliseconds(200)); + + auto req = make_shared(); + req->SetMethod(http::Method::GET); + req->SetAddress("http://127.0.0.1:" TEST_PORT); + + vector buf; + buf.resize(1235); + bool got_read_error {false}; + error::Error read_error; + + client->AsyncCall( + req, + [&](http::ExpectedIncomingResponsePtr exp_resp) { + ASSERT_TRUE(exp_resp) << exp_resp.error().String(); + auto resp = exp_resp.value(); + + auto reader = *client->MakeBodyAsyncReader(resp); + reader->RepeatedAsyncRead( + buf.begin(), + buf.end(), + // Capture `reader` to keep it alive. + [reader, &got_read_error, &read_error, &loop](io::ExpectedSize result) { + if (!result) { + got_read_error = true; + read_error = result.error(); + loop.Stop(); + return io::Repeat::No; + } + if (result.value() == 0) { + // Unexpected clean EOF; stop so the test does not hang. + loop.Stop(); + return io::Repeat::No; + } + return io::Repeat::Yes; + }); + }, + [](http::ExpectedIncomingResponsePtr exp_resp) { + // Body handler intentionally ignores errors, exactly like the update state + // machine: the error must surface through the reader instead. + }); + + loop.Run(); + + EXPECT_TRUE(got_read_error) + << "resumer gave up but the reader never reported an error (the daemon would hang here)"; + EXPECT_EQ(read_error.code, http::MakeError(http::DownloadResumerError, "").code) + << "unexpected error: " << read_error.String(); +} + TEST_F(DownloadResumerTest, TwoRangesClientReuse) { TestEventLoop loop(chrono::seconds(10));