From 6415830f1681dacd95485711488e511327db578c Mon Sep 17 00:00:00 2001 From: psiva01 Date: Fri, 3 Jul 2026 17:27:36 +0530 Subject: [PATCH 1/3] VPAAMP-698: Improve retry logic for CURL errors in AAMP Reason for change: Added centralized helper for CURL error handling and included all the transient curl failures typically treated as retryable. Signed-off-by: psiva01 --- downloader/AampCurlDownloader.cpp | 21 +++++- priv_aamp.cpp | 32 ++++++++- .../AampCurlDownloader/FunctionalTests.cpp | 68 +++++++++++++++++++ 3 files changed, 117 insertions(+), 4 deletions(-) diff --git a/downloader/AampCurlDownloader.cpp b/downloader/AampCurlDownloader.cpp index 4931e8dd09..82999a33ca 100644 --- a/downloader/AampCurlDownloader.cpp +++ b/downloader/AampCurlDownloader.cpp @@ -116,6 +116,24 @@ long aamp_CurlEasyGetinfoLong( CURL *handle, CURLINFO info ) return rc; } +static inline bool IsRetryableCurlFailure(CURLcode curlCode) +{ + switch (curlCode) + { + case CURLE_OPERATION_TIMEDOUT: + case CURLE_PARTIAL_FILE: + case CURLE_COULDNT_RESOLVE_PROXY: + case CURLE_COULDNT_RESOLVE_HOST: + case CURLE_COULDNT_CONNECT: + case CURLE_RECV_ERROR: + case CURLE_SEND_ERROR: + case CURLE_GOT_NOTHING: + return true; + default: + return false; + } +} + char *aamp_CurlEasyGetinfoString( CURL *handle, CURLINFO info ) { char *rc = NULL; @@ -368,7 +386,8 @@ int AampCurlDownloader::Download(const std::string &urlStr, std::shared_ptr(httpRetVal)) || + IsCurlTimeoutFailure(httpRetVal)) { AAMPLOG_WARN("Download failed due to curl error %d numDownloadAttempts %d numRetriesAllowed %d", httpRetVal, numDownloadAttempts, numRetriesAllowed); loopAgain = true; diff --git a/priv_aamp.cpp b/priv_aamp.cpp index 584f3eb142..46d748e148 100644 --- a/priv_aamp.cpp +++ b/priv_aamp.cpp @@ -4316,6 +4316,27 @@ static inline bool HasDownloadTimedOutWithData(CURLcode curlCode, CurlAbortReaso abortReason == eCURL_ABORT_REASON_LOW_BANDWIDTH_TIMEDOUT; } +/** + * @brief Check if curl failure is retryable based on transient transport errors + */ +static inline bool IsRetryableCurlFailure(CURLcode curlCode) +{ + switch (curlCode) + { + case CURLE_OPERATION_TIMEDOUT: + case CURLE_PARTIAL_FILE: + case CURLE_COULDNT_RESOLVE_PROXY: + case CURLE_COULDNT_RESOLVE_HOST: + case CURLE_COULDNT_CONNECT: + case CURLE_RECV_ERROR: + case CURLE_SEND_ERROR: + case CURLE_GOT_NOTHING: + return true; + default: + return false; + } +} + /** * @brief Parse a downloaded segment with a persistent per-track Mp4Demux to detect * structural corruption (any condition that triggers Mp4Demux::setParseError). @@ -4984,10 +5005,15 @@ bool PrivateInstanceAAMP::GetFile( std::string remoteUrl, AampMediaType mediaTyp print_headerResponse(context.allResponseHeaders, mediaType); } - if (res == CURLE_COULDNT_CONNECT || IsCurlTimeoutFailure(res) || + const bool isRetryableCurlError = IsRetryableCurlFailure(res); + const bool isRetryableTimeout = + (!isRetryableCurlError && IsCurlTimeoutFailure(res)); + const bool isRetryableStall = (isDownloadStalled && - (eCURL_ABORT_REASON_LOW_BANDWIDTH_TIMEDOUT != abortReason)) || - res == CURLE_SEND_ERROR) + (eCURL_ABORT_REASON_LOW_BANDWIDTH_TIMEDOUT != abortReason)); + + if (isRetryableCurlError || isRetryableTimeout || + isRetryableStall) { if(mpStreamAbstractionAAMP) diff --git a/test/utests/tests/AampCurlDownloader/FunctionalTests.cpp b/test/utests/tests/AampCurlDownloader/FunctionalTests.cpp index 010fbef860..a9301ec11f 100644 --- a/test/utests/tests/AampCurlDownloader/FunctionalTests.cpp +++ b/test/utests/tests/AampCurlDownloader/FunctionalTests.cpp @@ -590,6 +590,74 @@ TEST_F(FunctionalTests, AampCurlDownloader_Retry_SendError) EXPECT_FALSE(mAampCurlDownloader->IsDownloadActive()); } +class RetryableCurlFailureTests : public FunctionalTests, + public ::testing::WithParamInterface +{ +}; + +/** + * @brief Verify that newly retryable curl transport failures are retried. + */ +TEST_P(RetryableCurlFailureTests, + RetryableCurlFailure_RetryAndSucceed) +{ + DownloadResponsePtr respData = std::make_shared(); + DownloadConfigPtr inpData = std::make_shared(); + inpData->bNeedDownloadMetrics = true; + inpData->bIgnoreResponseHeader = true; + inpData->iDownloadRetryCount = 1; + + EXPECT_CALL(*g_mockCurl, curl_easy_init()).WillOnce(Return(mCurlEasyHandle)); + /* The curl easy handle will be cleaned when AampCurlDownloader is destroyed. */ + EXPECT_CALL(*g_mockCurl, curl_easy_cleanup(mCurlEasyHandle)); + EXPECT_CALL(*g_mockCurl, + curl_easy_setopt_ptr(mCurlEasyHandle, CURLOPT_PROGRESSDATA, + mAampCurlDownloader)).WillOnce(Return(CURLE_OK)); + EXPECT_CALL(*g_mockCurl, + curl_easy_setopt_func_xferinfo(mCurlEasyHandle, + CURLOPT_XFERINFOFUNCTION, NotNull())) + .WillOnce(DoAll(SaveArgPointee<2>(&mCurlProgressCallback), + Return(CURLE_OK))); + EXPECT_CALL(*g_mockCurl, + curl_easy_setopt_ptr(mCurlEasyHandle, CURLOPT_WRITEDATA, + mAampCurlDownloader)).WillOnce(Return(CURLE_OK)); + EXPECT_CALL(*g_mockCurl, + curl_easy_setopt_func_write(mCurlEasyHandle, + CURLOPT_WRITEFUNCTION, NotNull())) + .WillOnce(DoAll(SaveArgPointee<2>(&mCurlWriteFunc), + Return(CURLE_OK))); + mAampCurlDownloader->Initialize(inpData); + + ASSERT_NE(mCurlProgressCallback, nullptr); + ASSERT_NE(mCurlWriteFunc, nullptr); + + EXPECT_CALL(*g_mockCurl, + curl_easy_setopt_str(mCurlEasyHandle, CURLOPT_URL, mUrl.c_str())) + .WillOnce(Return(CURLE_OK)); + EXPECT_CALL(*g_mockCurl, curl_easy_perform(mCurlEasyHandle)) + .WillOnce(Return(GetParam())) + .WillOnce(Return(CURLE_OK)); + EXPECT_CALL(*g_mockCurl, + curl_easy_getinfo_int(mCurlEasyHandle, CURLINFO_RESPONSE_CODE, + NotNull())) + .WillOnce(DoAll(SetArgPointee<2>(200), Return(CURLE_OK))); + + mAampCurlDownloader->Download(mUrl, respData); + + EXPECT_EQ(200, respData->iHttpRetValue); + EXPECT_FALSE(mAampCurlDownloader->IsDownloadActive()); +} + +INSTANTIATE_TEST_SUITE_P( + RetryableTransportFailures, + RetryableCurlFailureTests, + ::testing::Values( + CURLE_PARTIAL_FILE, + CURLE_COULDNT_RESOLVE_PROXY, + CURLE_COULDNT_RESOLVE_HOST, + CURLE_RECV_ERROR, + CURLE_GOT_NOTHING)); + /** * @brief Verify that downloadCompleteMetrics.total uses cumulative wall-clock time, not CURLINFO_TOTAL_TIME. * From ee49b052a65de82791e19e0b88473c4e101c0190 Mon Sep 17 00:00:00 2001 From: psiva01 Date: Wed, 15 Jul 2026 16:35:37 +0530 Subject: [PATCH 2/3] VPAAMP-698: Address copilot review comments Signed-off-by: psiva01 --- AampUtils.cpp | 24 ++++++++++++++++++++++++ AampUtils.h | 2 ++ downloader/AampCurlDownloader.cpp | 19 ------------------- priv_aamp.cpp | 27 +-------------------------- 4 files changed, 27 insertions(+), 45 deletions(-) diff --git a/AampUtils.cpp b/AampUtils.cpp index 7e54aec70a..a39d6c8d3b 100644 --- a/AampUtils.cpp +++ b/AampUtils.cpp @@ -1546,3 +1546,27 @@ bool IsCurlTimeoutFailure( int httpResponseCode ) return false; } } + +/** + * @brief Check if a curl failure is retryable based on transient transport errors + * @param[in] curlCode curl error code + * @retval true if the error is retryable + * @retval false otherwise + */ +bool IsRetryableCurlFailure( CURLcode curlCode ) +{ + switch( curlCode ) + { + case CURLE_OPERATION_TIMEDOUT: + case CURLE_PARTIAL_FILE: + case CURLE_COULDNT_RESOLVE_PROXY: + case CURLE_COULDNT_RESOLVE_HOST: + case CURLE_COULDNT_CONNECT: + case CURLE_RECV_ERROR: + case CURLE_SEND_ERROR: + case CURLE_GOT_NOTHING: + return true; + default: + return false; + } +} diff --git a/AampUtils.h b/AampUtils.h index 9928523fe2..878142f260 100644 --- a/AampUtils.h +++ b/AampUtils.h @@ -469,4 +469,6 @@ CurlTimeoutFailureReason GetCurlTimeoutFailureReason(CURL* curl); bool IsCurlTimeoutFailure( int httpResponseCode ); +bool IsRetryableCurlFailure( CURLcode curlCode ); + #endif /* __AAMP_UTILS_H__ */ diff --git a/downloader/AampCurlDownloader.cpp b/downloader/AampCurlDownloader.cpp index 82999a33ca..f707e16bb3 100644 --- a/downloader/AampCurlDownloader.cpp +++ b/downloader/AampCurlDownloader.cpp @@ -115,25 +115,6 @@ long aamp_CurlEasyGetinfoLong( CURL *handle, CURLINFO info ) } return rc; } - -static inline bool IsRetryableCurlFailure(CURLcode curlCode) -{ - switch (curlCode) - { - case CURLE_OPERATION_TIMEDOUT: - case CURLE_PARTIAL_FILE: - case CURLE_COULDNT_RESOLVE_PROXY: - case CURLE_COULDNT_RESOLVE_HOST: - case CURLE_COULDNT_CONNECT: - case CURLE_RECV_ERROR: - case CURLE_SEND_ERROR: - case CURLE_GOT_NOTHING: - return true; - default: - return false; - } -} - char *aamp_CurlEasyGetinfoString( CURL *handle, CURLINFO info ) { char *rc = NULL; diff --git a/priv_aamp.cpp b/priv_aamp.cpp index 11e7af7535..b11f05f15a 100644 --- a/priv_aamp.cpp +++ b/priv_aamp.cpp @@ -4322,27 +4322,6 @@ static inline bool HasDownloadTimedOutWithData(CURLcode curlCode, CurlAbortReaso abortReason == eCURL_ABORT_REASON_LOW_BANDWIDTH_TIMEDOUT; } -/** - * @brief Check if curl failure is retryable based on transient transport errors - */ -static inline bool IsRetryableCurlFailure(CURLcode curlCode) -{ - switch (curlCode) - { - case CURLE_OPERATION_TIMEDOUT: - case CURLE_PARTIAL_FILE: - case CURLE_COULDNT_RESOLVE_PROXY: - case CURLE_COULDNT_RESOLVE_HOST: - case CURLE_COULDNT_CONNECT: - case CURLE_RECV_ERROR: - case CURLE_SEND_ERROR: - case CURLE_GOT_NOTHING: - return true; - default: - return false; - } -} - /** * @brief Parse a downloaded segment with a persistent per-track Mp4Demux to detect * structural corruption (any condition that triggers Mp4Demux::setParseError). @@ -5011,14 +4990,10 @@ bool PrivateInstanceAAMP::GetFile( std::string remoteUrl, AampMediaType mediaTyp print_headerResponse(context.allResponseHeaders, mediaType); } - const bool isRetryableCurlError = IsRetryableCurlFailure(res); - const bool isRetryableTimeout = - (!isRetryableCurlError && IsCurlTimeoutFailure(res)); const bool isRetryableStall = (isDownloadStalled && (eCURL_ABORT_REASON_LOW_BANDWIDTH_TIMEDOUT != abortReason)); - - if (isRetryableCurlError || isRetryableTimeout || + if (IsRetryableCurlFailure(res) || IsCurlTimeoutFailure(res) || isRetryableStall) { From e83e6c725e3ec267a696908a31678372189ce25a Mon Sep 17 00:00:00 2001 From: psiva01 Date: Wed, 15 Jul 2026 18:22:39 +0530 Subject: [PATCH 3/3] VPAAMP-698: L1 build fix Signed-off-by: psiva01 --- test/utests/fakes/FakeAampUtils.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/utests/fakes/FakeAampUtils.cpp b/test/utests/fakes/FakeAampUtils.cpp index a2c7f89856..b00c81b7f2 100644 --- a/test/utests/fakes/FakeAampUtils.cpp +++ b/test/utests/fakes/FakeAampUtils.cpp @@ -652,5 +652,9 @@ bool IsCurlTimeoutFailure( int httpResponseCode ) { return true; } +bool IsRetryableCurlFailure( CURLcode curlCode ) +{ + return true; +} // aamp_ApplyPageHttpHeaders not actually part of AampUtils.cpp, but fake declared here for convenience extern "C" void aamp_ApplyPageHttpHeaders(PlayerInstanceAAMP *aamp){}