Drop alternative URIs we got a hash-based fail from

If we got a file but it produced a hash error, mismatched size or
similar we shouldn't fallback to alternative URIs as they likely result
in the same error. If we can we should instead use another mirror.

We used to be a lot stricter by stopping all trys for this file if we
got a non-404 (or a hash-based) failure, but that is too hard as we
really want to try other mirrors (if we have them) in the hope that they
have the expected and correct files.

5 files changed, 117 insertions, 104 deletions

diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc
index f5986a260..d0b92ec66 100644
--- a/apt-pkg/acquire-item.cc
+++ b/apt-pkg/acquire-item.cc
@@ -399,18 +399,23 @@ bool pkgAcqTransactionItem::QueueURI(pkgAcquire::ItemDesc &Item)
       return false;
    }
    // If we got the InRelease file via a mirror, pick all indexes directly from this mirror, too
-   if (TransactionManager->BaseURI.empty() == false && UsedMirror.empty() &&
-	 URI::SiteOnly(Item.URI) != URI::SiteOnly(TransactionManager->BaseURI))
+   if (TransactionManager->BaseURI.empty() == false && TransactionManager->UsedMirror.empty() == false &&
+       URI::SiteOnly(Item.URI) != URI::SiteOnly(TransactionManager->BaseURI))
    {
       // this ensures we rewrite only once and only the first step
       auto const OldBaseURI = Target.Option(IndexTarget::BASE_URI);
       if (OldBaseURI.empty() == false && APT::String::Startswith(Item.URI, OldBaseURI))
       {
 	 auto const ExtraPath = Item.URI.substr(OldBaseURI.length());
-	 Item.URI = flCombine(TransactionManager->BaseURI, ExtraPath);
-	 UsedMirror = TransactionManager->UsedMirror;
-	 if (Item.Description.find(" ") != string::npos)
-	    Item.Description.replace(0, Item.Description.find(" "), UsedMirror);
+	 auto newURI = flCombine(TransactionManager->BaseURI, ExtraPath);
+	 if (IsGoodAlternativeURI(newURI))
+	 {
+	    PushAlternativeURI(std::string(Item.URI), {}, false);
+	    Item.URI = std::move(newURI);
+	    UsedMirror = TransactionManager->UsedMirror;
+	    if (Item.Description.find(" ") != string::npos)
+	       Item.Description.replace(0, Item.Description.find(" "), UsedMirror);
+	 }
       }
    }
    return pkgAcquire::Item::QueueURI(Item);
@@ -686,11 +691,12 @@ class pkgAcquire::Item::Private
 public:
    struct AlternateURI
    {
-      std::string const URI;
+      std::string URI;
       std::unordered_map<std::string, std::string> changefields;
       AlternateURI(std::string &&u, decltype(changefields) &&cf) : URI(u), changefields(cf) {}
    };
    std::list<AlternateURI> AlternativeURIs;
+   std::vector<std::string> BadAlternativeSites;
    std::vector<std::string> PastRedirections;
    std::unordered_map<std::string, std::string> CustomFields;
    unsigned int Retries;
@@ -749,14 +755,32 @@ bool pkgAcquire::Item::PopAlternativeURI(std::string &NewURI) /*{{{*/
    return true;
 }
 									/*}}}*/
+bool pkgAcquire::Item::IsGoodAlternativeURI(std::string const &AltUri) const/*{{{*/
+{
+   return std::find(d->PastRedirections.cbegin(), d->PastRedirections.cend(), AltUri) == d->PastRedirections.cend() &&
+	 std::find(d->BadAlternativeSites.cbegin(), d->BadAlternativeSites.cend(), URI::SiteOnly(AltUri)) == d->BadAlternativeSites.cend();
+}
+									/*}}}*/
 void pkgAcquire::Item::PushAlternativeURI(std::string &&NewURI, std::unordered_map<std::string, std::string> &&fields, bool const at_the_back) /*{{{*/
 {
+   if (IsGoodAlternativeURI(NewURI) == false)
+      return;
    if (at_the_back)
       d->AlternativeURIs.emplace_back(std::move(NewURI), std::move(fields));
    else
       d->AlternativeURIs.emplace_front(std::move(NewURI), std::move(fields));
 }
 									/*}}}*/
+void pkgAcquire::Item::RemoveAlternativeSite(std::string &&OldSite) /*{{{*/
+{
+   d->AlternativeURIs.erase(std::remove_if(d->AlternativeURIs.begin(), d->AlternativeURIs.end(),
+					   [&](decltype(*d->AlternativeURIs.cbegin()) AltUri) {
+					      return URI::SiteOnly(AltUri.URI) == OldSite;
+					   }),
+			    d->AlternativeURIs.end());
+   d->BadAlternativeSites.push_back(std::move(OldSite));
+}
+									/*}}}*/
 unsigned int &pkgAcquire::Item::ModifyRetries() /*{{{*/
 {
    return d->Retries;
@@ -2231,7 +2255,7 @@ void pkgAcqDiffIndex::QueueOnIMSHit() const				/*{{{*/
 {
    // list cleanup needs to know that this file as well as the already
    // present index is ours, so we create an empty diff to save it for us
-   new pkgAcqIndexDiffs(Owner, TransactionManager, Target, UsedMirror, Target.URI);
+   new pkgAcqIndexDiffs(Owner, TransactionManager, Target, Target.URI);
 }
 									/*}}}*/
 static bool RemoveFileForBootstrapLinking(std::string &ErrorText, std::string const &For, std::string const &Boot)/*{{{*/
@@ -2585,7 +2609,7 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile)	/*{{{*/
 									/*}}}*/
 void pkgAcqDiffIndex::Failed(string const &Message,pkgAcquire::MethodConfig const * const Cnf)/*{{{*/
 {
-   if (CommonFailed(GetDiffIndexURI(Target), GetDiffIndexFileName(Target.Description), Message, Cnf))
+   if (CommonFailed(GetDiffIndexURI(Target), Message, Cnf))
       return;
 
    RenameOnError(PDiffError);
@@ -2649,15 +2673,15 @@ void pkgAcqDiffIndex::Done(string const &Message,HashStringList const &Hashes,	/
       }
 
       if (pdiff_merge == false)
-	 new pkgAcqIndexDiffs(Owner, TransactionManager, Target, UsedMirror, indexURI, available_patches);
+	 new pkgAcqIndexDiffs(Owner, TransactionManager, Target, indexURI, available_patches);
       else
       {
 	 diffs = new std::vector<pkgAcqIndexMergeDiffs*>(available_patches.size());
 	 for(size_t i = 0; i < available_patches.size(); ++i)
 	    (*diffs)[i] = new pkgAcqIndexMergeDiffs(Owner, TransactionManager,
-		  Target, UsedMirror, indexURI,
-		  available_patches[i],
-		  diffs);
+						    Target, indexURI,
+						    available_patches[i],
+						    diffs);
       }
    }
 
@@ -2681,13 +2705,13 @@ pkgAcqDiffIndex::~pkgAcqDiffIndex()
 /* The package diff is added to the queue. one object is constructed
  * for each diff and the index
  */
-pkgAcqIndexDiffs::pkgAcqIndexDiffs(pkgAcquire * const Owner,
-                                   pkgAcqMetaClearSig * const TransactionManager,
-                                   IndexTarget const &Target,
-				   std::string const &indexUsedMirror, std::string const &indexURI,
+pkgAcqIndexDiffs::pkgAcqIndexDiffs(pkgAcquire *const Owner,
+				   pkgAcqMetaClearSig *const TransactionManager,
+				   IndexTarget const &Target,
+				   std::string const &indexURI,
 				   vector<DiffInfo> const &diffs)
-   : pkgAcqBaseIndex(Owner, TransactionManager, Target), indexURI(indexURI),
-     available_patches(diffs)
+    : pkgAcqBaseIndex(Owner, TransactionManager, Target), indexURI(indexURI),
+      available_patches(diffs)
 {
    DestFile = GetKeepCompressedFileName(GetPartialFileNameFromURI(Target.URI), Target);
 
@@ -2697,12 +2721,6 @@ pkgAcqIndexDiffs::pkgAcqIndexDiffs(pkgAcquire * const Owner,
    Description = Target.Description;
    Desc.ShortDesc = Target.ShortDesc;
 
-   UsedMirror = indexUsedMirror;
-   if (UsedMirror == "DIRECT")
-      UsedMirror.clear();
-   else if (UsedMirror.empty() == false && Description.find(" ") != string::npos)
-      Description.replace(0, Description.find(" "), UsedMirror);
-
    if(available_patches.empty() == true)
    {
       // we are done (yeah!), check hashes against the final file
@@ -2870,7 +2888,7 @@ void pkgAcqIndexDiffs::Done(string const &Message, HashStringList const &Hashes,
 	 // see if there is more to download
 	 if(available_patches.empty() == false)
 	 {
-	    new pkgAcqIndexDiffs(Owner, TransactionManager, Target, UsedMirror, indexURI, available_patches);
+	    new pkgAcqIndexDiffs(Owner, TransactionManager, Target, indexURI, available_patches);
 	    Finish();
 	 } else {
 	    DestFile = PatchedFile;
@@ -2896,23 +2914,17 @@ std::string pkgAcqIndexDiffs::Custom600Headers() const			/*{{{*/
 pkgAcqIndexDiffs::~pkgAcqIndexDiffs() {}
 
 // AcqIndexMergeDiffs::AcqIndexMergeDiffs - Constructor			/*{{{*/
-pkgAcqIndexMergeDiffs::pkgAcqIndexMergeDiffs(pkgAcquire * const Owner,
-                                             pkgAcqMetaClearSig * const TransactionManager,
-                                             IndexTarget const &Target,
-					     std::string const &indexUsedMirror, std::string const &indexURI,
-                                             DiffInfo const &patch,
-                                             std::vector<pkgAcqIndexMergeDiffs*> const * const allPatches)
-  : pkgAcqBaseIndex(Owner, TransactionManager, Target), indexURI(indexURI),
-    patch(patch), allPatches(allPatches), State(StateFetchDiff)
+pkgAcqIndexMergeDiffs::pkgAcqIndexMergeDiffs(pkgAcquire *const Owner,
+					     pkgAcqMetaClearSig *const TransactionManager,
+					     IndexTarget const &Target,
+					     std::string const &indexURI,
+					     DiffInfo const &patch,
+					     std::vector<pkgAcqIndexMergeDiffs *> const *const allPatches)
+    : pkgAcqBaseIndex(Owner, TransactionManager, Target), indexURI(indexURI),
+      patch(patch), allPatches(allPatches), State(StateFetchDiff)
 {
    Debug = _config->FindB("Debug::pkgAcquire::Diffs",false);
-
    Description = Target.Description;
-   UsedMirror = indexUsedMirror;
-   if (UsedMirror == "DIRECT")
-      UsedMirror.clear();
-   else if (UsedMirror.empty() == false && Description.find(" ") != string::npos)
-      Description.replace(0, Description.find(" "), UsedMirror);
 
    Desc.Owner = this;
    Desc.ShortDesc = Target.ShortDesc;
@@ -3170,24 +3182,10 @@ string pkgAcqIndex::Custom600Headers() const
 }
 									/*}}}*/
 // AcqIndex::Failed - getting the indexfile failed			/*{{{*/
-bool pkgAcqIndex::CommonFailed(std::string const &TargetURI, std::string const TargetDesc,
-      std::string const &Message, pkgAcquire::MethodConfig const * const Cnf)
+bool pkgAcqIndex::CommonFailed(std::string const &TargetURI,
+			       std::string const &Message, pkgAcquire::MethodConfig const *const Cnf)
 {
    pkgAcqBaseIndex::Failed(Message,Cnf);
-
-   if (UsedMirror.empty() == false && UsedMirror != "DIRECT" &&
-	 LookupTag(Message, "FailReason") == "HttpError404")
-   {
-      UsedMirror = "DIRECT";
-      if (Desc.URI.find("/by-hash/") != std::string::npos)
-	 CompressionExtensions = "by-hash " + CompressionExtensions;
-      else
-	 CompressionExtensions = CurrentCompressionExtension + ' ' + CompressionExtensions;
-      Init(TargetURI, TargetDesc, Desc.ShortDesc);
-      Status = StatIdle;
-      return true;
-   }
-
    // authorisation matches will not be fixed by other compression types
    if (Status != StatAuthError)
    {
@@ -3202,7 +3200,7 @@ bool pkgAcqIndex::CommonFailed(std::string const &TargetURI, std::string const T
 }
 void pkgAcqIndex::Failed(string const &Message,pkgAcquire::MethodConfig const * const Cnf)
 {
-   if (CommonFailed(Target.URI, Target.Description, Message, Cnf))
+   if (CommonFailed(Target.URI, Message, Cnf))
       return;
 
    if(Target.IsOptional && GetExpectedHashes().empty() && Stage == STAGE_DOWNLOAD)
diff --git a/apt-pkg/acquire-item.h b/apt-pkg/acquire-item.h
index 46d79df92..814596850 100644
--- a/apt-pkg/acquire-item.h
+++ b/apt-pkg/acquire-item.h
@@ -246,7 +246,9 @@ class pkgAcquire::Item : public WeakPointable				/*{{{*/
    APT_HIDDEN std::unordered_map<std::string, std::string> &ModifyCustomFields();
    // this isn't the super nicest interface either…
    APT_HIDDEN bool PopAlternativeURI(std::string &NewURI);
+   APT_HIDDEN bool IsGoodAlternativeURI(std::string const &AltUri) const;
    APT_HIDDEN void PushAlternativeURI(std::string &&NewURI, std::unordered_map<std::string, std::string> &&fields, bool const at_the_back);
+   APT_HIDDEN void RemoveAlternativeSite(std::string &&OldSite);
 
    /** \brief A "descriptive" URI-like string.
     *
@@ -690,8 +692,8 @@ class APT_HIDDEN pkgAcqIndex : public pkgAcqBaseIndex
    protected:
    APT_HIDDEN void Init(std::string const &URI, std::string const &URIDesc,
              std::string const &ShortDesc);
-   APT_HIDDEN bool CommonFailed(std::string const &TargetURI, std::string const TargetDesc,
-             std::string const &Message, pkgAcquire::MethodConfig const * const Cnf);
+   APT_HIDDEN bool CommonFailed(std::string const &TargetURI,
+				std::string const &Message, pkgAcquire::MethodConfig const *const Cnf);
 };
 									/*}}}*/
 struct APT_HIDDEN DiffInfo {						/*{{{*/
@@ -850,10 +852,10 @@ class APT_HIDDEN pkgAcqIndexMergeDiffs : public pkgAcqBaseIndex
     *  \param allPatches contains all related items so that each item can
     *  check if it was the last one to complete the download step
     */
-   pkgAcqIndexMergeDiffs(pkgAcquire * const Owner, pkgAcqMetaClearSig * const TransactionManager,
-			 IndexTarget const &Target, std::string const &indexUsedMirror,
+   pkgAcqIndexMergeDiffs(pkgAcquire *const Owner, pkgAcqMetaClearSig *const TransactionManager,
+			 IndexTarget const &Target,
 			 std::string const &indexURI, DiffInfo const &patch,
-                         std::vector<pkgAcqIndexMergeDiffs*> const * const allPatches) APT_NONNULL(2, 3, 8);
+			 std::vector<pkgAcqIndexMergeDiffs *> const *const allPatches) APT_NONNULL(2, 3, 7);
    virtual ~pkgAcqIndexMergeDiffs();
 };
 									/*}}}*/
@@ -957,10 +959,10 @@ class APT_HIDDEN pkgAcqIndexDiffs : public pkgAcqBaseIndex
     *  should be ordered so that each diff appears before any diff
     *  that depends on it.
     */
-   pkgAcqIndexDiffs(pkgAcquire * const Owner, pkgAcqMetaClearSig * const TransactionManager,
-                    IndexTarget const &Target,
-		    std::string const &indexUsedMirror, std::string const &indexURI,
-		    std::vector<DiffInfo> const &diffs=std::vector<DiffInfo>()) APT_NONNULL(2, 3);
+   pkgAcqIndexDiffs(pkgAcquire *const Owner, pkgAcqMetaClearSig *const TransactionManager,
+		    IndexTarget const &Target,
+		    std::string const &indexURI,
+		    std::vector<DiffInfo> const &diffs = std::vector<DiffInfo>()) APT_NONNULL(2, 3);
    virtual ~pkgAcqIndexDiffs();
 };
 									/*}}}*/
diff --git a/apt-pkg/acquire-worker.cc b/apt-pkg/acquire-worker.cc
index d159ef84f..c2bbf8bed 100644
--- a/apt-pkg/acquire-worker.cc
+++ b/apt-pkg/acquire-worker.cc
@@ -321,28 +321,35 @@ bool pkgAcquire::Worker::RunMessages()
 	    Itm = nullptr;
 	    for (auto const &Owner: ItmOwners)
 	    {
+	       for (auto alt = AltUris.crbegin(); alt != AltUris.crend(); ++alt)
+		  Owner->PushAlternativeURI(std::string(*alt), {}, false);
+
 	       pkgAcquire::ItemDesc &desc = Owner->GetItemDesc();
-	       if (Owner->IsRedirectionLoop(NewURI))
+	       // for a simplified retry a method might redirect without URI change
+	       // see also IsRedirectionLoop implementation
+	       if (desc.URI != NewURI)
 	       {
-		  std::string msg = Message;
-		  msg.append("\nFailReason: RedirectionLoop");
-		  Owner->Failed(msg, Config);
-		  if (Log != nullptr)
-		     Log->Fail(Owner->GetItemDesc());
-		  continue;
-	       }
+		  auto newuri = NewURI;
+		  if (Owner->IsGoodAlternativeURI(newuri) == false && Owner->PopAlternativeURI(newuri) == false)
+		     newuri.clear();
+		  if (newuri.empty() || Owner->IsRedirectionLoop(newuri))
+		  {
+		     std::string msg = Message;
+		     msg.append("\nFailReason: RedirectionLoop");
+		     Owner->Failed(msg, Config);
+		     if (Log != nullptr)
+			Log->Fail(Owner->GetItemDesc());
+		     continue;
+		  }
 
-	       if (Log != nullptr)
-		  Log->Done(desc);
+		  if (Log != nullptr)
+		     Log->Done(desc);
 
-	       ChangeSiteIsMirrorChange(NewURI, desc, Owner);
-	       desc.URI = NewURI;
+		  ChangeSiteIsMirrorChange(NewURI, desc, Owner);
+		  desc.URI = NewURI;
+	       }
 	       if (isDoomedItem(Owner) == false)
-	       {
-		  for (auto alt = AltUris.crbegin(); alt != AltUris.crend(); ++alt)
-		     Owner->PushAlternativeURI(std::string(*alt), {}, false);
 		  OwnerQ->Owner->Enqueue(desc);
-	       }
 	    }
             break;
          }
@@ -608,28 +615,33 @@ void pkgAcquire::Worker::HandleFailure(std::vector<pkgAcquire::Item *> const &It
 	 if (isDoomedItem(Owner) == false)
 	    OwnerQ->Owner->Enqueue(SavedDesc);
       }
-      else if (Owner->PopAlternativeURI(NewURI))
-      {
-	 Owner->FailMessage(Message);
-	 auto &desc = Owner->GetItemDesc();
-	 if (Log != nullptr)
-	    Log->Fail(desc);
-	 ChangeSiteIsMirrorChange(NewURI, desc, Owner);
-	 desc.URI = NewURI;
-	 if (isDoomedItem(Owner) == false)
-	    OwnerQ->Owner->Enqueue(desc);
-      }
       else
       {
-	 if (errAuthErr && Owner->GetExpectedHashes().empty() == false)
-	    Owner->Status = pkgAcquire::Item::StatAuthError;
-	 else if (errTransient)
-	    Owner->Status = pkgAcquire::Item::StatTransientNetworkError;
-	 auto SavedDesc = Owner->GetItemDesc();
-	 if (isDoomedItem(Owner) == false)
-	    Owner->Failed(Message, Config);
-	 if (Log != nullptr)
-	    Log->Fail(SavedDesc);
+	 if (errAuthErr)
+	    Owner->RemoveAlternativeSite(URI::SiteOnly(Owner->GetItemDesc().URI));
+	 if (Owner->PopAlternativeURI(NewURI))
+	 {
+	    Owner->FailMessage(Message);
+	    auto &desc = Owner->GetItemDesc();
+	    if (Log != nullptr)
+	       Log->Fail(desc);
+	    ChangeSiteIsMirrorChange(NewURI, desc, Owner);
+	    desc.URI = NewURI;
+	    if (isDoomedItem(Owner) == false)
+	       OwnerQ->Owner->Enqueue(desc);
+	 }
+	 else
+	 {
+	    if (errAuthErr && Owner->GetExpectedHashes().empty() == false)
+	       Owner->Status = pkgAcquire::Item::StatAuthError;
+	    else if (errTransient)
+	       Owner->Status = pkgAcquire::Item::StatTransientNetworkError;
+	    auto SavedDesc = Owner->GetItemDesc();
+	    if (isDoomedItem(Owner) == false)
+	       Owner->Failed(Message, Config);
+	    if (Log != nullptr)
+	       Log->Fail(SavedDesc);
+	 }
       }
    }
 }
diff --git a/test/integration/test-partial-file-support b/test/integration/test-partial-file-support
index 9b5eed1e5..88fa91324 100755
--- a/test/integration/test-partial-file-support
+++ b/test/integration/test-partial-file-support
@@ -24,7 +24,7 @@ testdownloadfile() {
 	else
 		msgpass
 	fi
-	sed -e '/^ <- / s#%20# #g' -e '/^ <- / s#%0a#\n#g' "$DOWNLOADLOG" | grep '^.*-Hash: ' > receivedhashes.log
+	sed -e '/^ <- / s#%20# #g' -e '/^ <- / s#%0a#\n#g' "$DOWNLOADLOG" | grep '^.*-Hash: ' > receivedhashes.log || true
 	testsuccess test -s receivedhashes.log
 	local HASHES_OK=0
 	local HASHES_BAD=0
diff --git a/test/integration/test-pdiff-usage b/test/integration/test-pdiff-usage
index 5a650ad83..eec384573 100755
--- a/test/integration/test-pdiff-usage
+++ b/test/integration/test-pdiff-usage
@@ -398,7 +398,8 @@ testcase -o Acquire::IndexTargets::deb::Packages::KeepCompressed=true
 
 
 partialleftovers() { generatepartialleftovers "redirectme_Packages.${LOWCOSTEXT}" "redirectme_Packages-patched.${LOWCOSTEXT}"; }
-webserverconfig 'aptwebserver::redirect::replace::/redirectme/' "http://0.0.0.0:${APTHTTPPORT}/"
+# redirect the InRelease file only – the other files are auto-redirected by apt
+webserverconfig 'aptwebserver::redirect::replace::/redirectme/I' "http://0.0.0.0:${APTHTTPPORT}/I"
 rewritesourceslist "http://localhost:${APTHTTPPORT}/redirectme"
 aptautotest_apt_update() {
 	aptautotest_aptget_update "$@"