improve partial/ cleanup in abort and failure cases

Especially pdiff-enhanced downloads have the tendency to fail for
various reasons from which we can recover and even a successful download
used to leave the old unpatched index in partial/.

By adding a new method responsible for making the transaction of an
individual file happen we can at specialisations especially for abort
cases to deal with the cleanup.

This also helps in keeping the compressed indexes around if another
index failed instead of keeping the decompressed files, which we
wouldn't pick up in the next call.

apt-pkg/acquire-item.cc

@@ -173,6 +173,39 @@ void pkgAcquire::Item::Failed(string Message,pkgAcquire::MethodConfig *Cnf)
       ReportMirrorFailure(ErrorText);
 }
 									/*}}}*/
+bool pkgAcquire::Item::TransactionState(TransactionStates const state)	/*{{{*/
+{
+   bool const Debug = _config->FindB("Debug::Acquire::Transaction", false);
+   switch(state)
+   {
+      case TransactionAbort:
+	 if(Debug == true)
+	    std::clog << "  Cancel: " << DestFile << std::endl;
+	 if (Status == pkgAcquire::Item::StatIdle)
+	 {
+	    Status = pkgAcquire::Item::StatDone;
+	    Dequeue();
+	 }
+	 break;
+      case TransactionCommit:
+	 if(PartialFile != "")
+	 {
+	    if(Debug == true)
+	       std::clog << "mv " << PartialFile << " -> "<< DestFile << " # " << DescURI() << std::endl;
+
+	    Rename(PartialFile, DestFile);
+	 } else {
+	    if(Debug == true)
+	       std::clog << "rm " << DestFile << " # " << DescURI() << std::endl;
+	    unlink(DestFile.c_str());
+	 }
+	 // mark that this transaction is finished
+	 TransactionManager = 0;
+	 break;
+   }
+   return true;
+}
+									/*}}}*/
 // Acquire::Item::Start - Item has begun to download			/*{{{*/
 // ---------------------------------------------------------------------
 /* Stash status and the file size. Note that setting Complete means 
@@ -300,6 +333,9 @@ bool pkgAcquire::Item::RenameOnError(pkgAcquire::Item::RenameOnErrorState const
 	 // the method is expected to report a good error for this
 	 Status = StatError;
 	 break;
+      case PDiffError:
+	 // no handling here, done by callers
+	 break;
    }
    return false;
 }
@@ -374,7 +410,7 @@ pkgAcqDiffIndex::pkgAcqDiffIndex(pkgAcquire *Owner,
 				 HashStringList const &ExpectedHashes,
                                  indexRecords *MetaIndexParser)
    : pkgAcqBaseIndex(Owner, TransactionManager, Target, ExpectedHashes, 
-                     MetaIndexParser), PackagesFileReadyInPartial(false)
+                     MetaIndexParser)
 {
    
    Debug = _config->FindB("Debug::pkgAcquire::Diffs",false);
@@ -671,20 +707,6 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string IndexDiffFile)		/*{{{*/
       return false;
    }
 
-   // FIXME: make this use the method
-   PackagesFileReadyInPartial = true;
-   std::string const Partial = GetPartialFileNameFromURI(RealURI);
-   
-   FileFd From(CurrentPackagesFile, FileFd::ReadOnly);
-   FileFd To(Partial, FileFd::WriteEmpty);
-   if(CopyFile(From, To) == false)
-      return _error->Errno("CopyFile", "failed to copy");
-   
-   if(Debug)
-      std::cerr << "Done copying " << CurrentPackagesFile
-                << " -> " << Partial
-                << std::endl;
-
    // we have something, queue the diffs
    string::size_type const last_space = Description.rfind(" ");
    if(last_space != string::npos)
@@ -738,6 +760,24 @@ void pkgAcqDiffIndex::Failed(string Message,pkgAcquire::MethodConfig * Cnf)/*{{{
    new pkgAcqIndex(Owner, TransactionManager, Target, ExpectedHashes, MetaIndexParser);
 }
 									/*}}}*/
+bool pkgAcqDiffIndex::TransactionState(TransactionStates const state)	/*{{{*/
+{
+   if (pkgAcquire::Item::TransactionState(state) == false)
+      return false;
+
+   switch (state)
+   {
+      case TransactionCommit:
+	 break;
+      case TransactionAbort:
+	 std::string const Partial = GetPartialFileNameFromURI(RealURI);
+	 unlink(Partial.c_str());
+	 break;
+   }
+
+   return true;
+}
+									/*}}}*/
 void pkgAcqDiffIndex::Done(string Message,unsigned long long Size,HashStringList const &Hashes,	/*{{{*/
 			   pkgAcquire::MethodConfig *Cnf)
 {
@@ -765,15 +805,21 @@ void pkgAcqDiffIndex::Done(string Message,unsigned long long Size,HashStringList
    if(StringToBool(LookupTag(Message,"IMS-Hit"),false))
       DestFile = FinalFile;
 
-   if(!ParseDiffIndex(DestFile))
-      return Failed("Message: Couldn't parse pdiff index", Cnf);
+   if(ParseDiffIndex(DestFile) == false)
+   {
+      Failed("Message: Couldn't parse pdiff index", Cnf);
+      // queue for final move - this should happen even if we fail
+      // while parsing (e.g. on sizelimit) and download the complete file.
+      TransactionManager->TransactionStageCopy(this, DestFile, FinalFile);
+      return;
+   }
 
-   // queue for final move
    TransactionManager->TransactionStageCopy(this, DestFile, FinalFile);
 
    Complete = true;
    Status = StatDone;
    Dequeue();
+
    return;
 }
 									/*}}}*/
@@ -808,6 +854,17 @@ pkgAcqIndexDiffs::pkgAcqIndexDiffs(pkgAcquire *Owner,
    }
    else
    {
+      // patching needs to be bootstrapped with the 'old' version
+      std::string const PartialFile = GetPartialFileNameFromURI(RealURI);
+      if (RealFileExists(PartialFile) == false)
+      {
+	 if (symlink(GetFinalFilename().c_str(), PartialFile.c_str()) != 0)
+	 {
+	    Failed("Link creation of " + PartialFile + " to " + GetFinalFilename() + " failed", NULL);
+	    return;
+	 }
+      }
+
       // get the next diff
       State = StateFetchDiff;
       QueueNextDiff();
@@ -822,6 +879,8 @@ void pkgAcqIndexDiffs::Failed(string Message,pkgAcquire::MethodConfig * Cnf)/*{{
    if(Debug)
       std::clog << "pkgAcqIndexDiffs failed: " << Desc.URI << " with " << Message << std::endl
 		<< "Falling back to normal index file acquire" << std::endl;
+   DestFile = GetPartialFileNameFromURI(Target->URI);
+   RenameOnError(PDiffError);
    new pkgAcqIndex(Owner, TransactionManager, Target, ExpectedHashes, MetaIndexParser);
    Finish();
 }
@@ -950,6 +1009,8 @@ void pkgAcqIndexDiffs::Done(string Message,unsigned long long Size, HashStringLi
       if (fd.Size() != available_patches[0].patch_size ||
 	    available_patches[0].patch_hashes != LocalHashes)
       {
+	 // patchfiles are dated, so bad indicates a bad download, so kill it
+	 unlink(DestFile.c_str());
 	 Failed("Patch has Size/Hashsum mismatch", NULL);
 	 return;
       }
@@ -1046,6 +1107,8 @@ void pkgAcqIndexMergeDiffs::Failed(string Message,pkgAcquire::MethodConfig * Cnf
    State = StateErrorDiff;
    if (Debug)
       std::clog << "Falling back to normal index file acquire" << std::endl;
+   DestFile = GetPartialFileNameFromURI(Target->URI);
+   RenameOnError(PDiffError);
    new pkgAcqIndex(Owner, TransactionManager, Target, ExpectedHashes, MetaIndexParser);
 }
 									/*}}}*/
@@ -1069,6 +1132,8 @@ void pkgAcqIndexMergeDiffs::Done(string Message,unsigned long long Size,HashStri
 
       if (fd.Size() != patch.patch_size || patch.patch_hashes != LocalHashes)
       {
+	 // patchfiles are dated, so bad indicates a bad download, so kill it
+	 unlink(DestFile.c_str());
 	 Failed("Patch has Size/Hashsum mismatch", NULL);
 	 return;
       }
@@ -1090,6 +1155,13 @@ void pkgAcqIndexMergeDiffs::Done(string Message,unsigned long long Size,HashStri
       // this is the last completed diff, so we are ready to apply now
       State = StateApplyDiff;
 
+      // patching needs to be bootstrapped with the 'old' version
+      if (symlink(GetFinalFilename().c_str(), FinalFile.c_str()) != 0)
+      {
+	 Failed("Link creation of " + FinalFile + " to " + GetFinalFilename() + " failed", NULL);
+	 return;
+      }
+
       if(Debug)
 	 std::clog << "Sending to rred method: " << FinalFile << std::endl;
 
@@ -1109,15 +1181,14 @@ void pkgAcqIndexMergeDiffs::Done(string Message,unsigned long long Size,HashStri
 	 return;
       }
 
-
       // move the result into place
-      std::string const FinalFile = GetFinalFilename();
+      std::string const Final = GetFinalFilename();
       if(Debug)
 	 std::clog << "Queue patched file in place: " << std::endl
-		   << DestFile << " -> " << FinalFile << std::endl;
+		   << DestFile << " -> " << Final << std::endl;
 
       // queue for copy by the transaction manager
-      TransactionManager->TransactionStageCopy(this, DestFile, FinalFile);
+      TransactionManager->TransactionStageCopy(this, DestFile, Final);
 
       // ensure the ed's are gone regardless of list-cleanup
       for (std::vector<pkgAcqIndexMergeDiffs *>::const_iterator I = allPatches->begin();
@@ -1127,6 +1198,7 @@ void pkgAcqIndexMergeDiffs::Done(string Message,unsigned long long Size,HashStri
 	 std::string patch = PartialFile + ".ed." + (*I)->patch.file + ".gz";
 	 unlink(patch.c_str());
       }
+      unlink(FinalFile.c_str());
 
       // all set and done
       Complete = true;
@@ -1337,12 +1409,6 @@ void pkgAcqIndex::Failed(string Message,pkgAcquire::MethodConfig *Cnf)
       return;
    }
 
-   // on decompression failure, remove bad versions in partial/
-   if (Stage == STAGE_DECOMPRESS_AND_VERIFY)
-   {
-      unlink(EraseFileName.c_str());
-   }
-
    Item::Failed(Message,Cnf);
 
    if(Target->IsOptional() && ExpectedHashes.empty() && Stage == STAGE_DOWNLOAD)
@@ -1351,6 +1417,30 @@ void pkgAcqIndex::Failed(string Message,pkgAcquire::MethodConfig *Cnf)
       TransactionManager->AbortTransaction();
 }
 									/*}}}*/
+bool pkgAcqIndex::TransactionState(TransactionStates const state)	/*{{{*/
+{
+   if (pkgAcquire::Item::TransactionState(state) == false)
+      return false;
+
+   switch (state)
+   {
+      case TransactionAbort:
+	 if (Stage == STAGE_DECOMPRESS_AND_VERIFY)
+	 {
+	    // keep the compressed file, but drop the decompressed
+	    EraseFileName.clear();
+	    if (PartialFile.empty() == false && flExtension(PartialFile) == "decomp")
+	       unlink(PartialFile.c_str());
+	 }
+	 break;
+      case TransactionCommit:
+	 if (EraseFileName.empty() == false)
+	    unlink(EraseFileName.c_str());
+	 break;
+   }
+   return true;
+}
+									/*}}}*/
 // pkgAcqIndex::GetFinalFilename - Return the full final file path	/*{{{*/
 std::string pkgAcqIndex::GetFinalFilename() const
 {
@@ -1530,9 +1620,6 @@ void pkgAcqIndex::StageDecompressDone(string Message,
       return;
    }
 
-   // remove the compressed version of the file
-   unlink(EraseFileName.c_str());
-
    // Done, queue for rename on transaction finished
    TransactionManager->TransactionStageCopy(this, DestFile, GetFinalFilename());
 
@@ -1568,14 +1655,7 @@ void pkgAcqMetaBase::AbortTransaction()
    for (std::vector<Item*>::iterator I = Transaction.begin();
         I != Transaction.end(); ++I)
    {
-      if(_config->FindB("Debug::Acquire::Transaction", false) == true)
-         std::clog << "  Cancel: " << (*I)->DestFile << std::endl;
-      // the transaction will abort, so stop anything that is idle
-      if ((*I)->Status == pkgAcquire::Item::StatIdle)
-      {
-         (*I)->Status = pkgAcquire::Item::StatDone;
-         (*I)->Dequeue();
-      }
+      (*I)->TransactionState(TransactionAbort);
    }
    Transaction.clear();
 }
@@ -1585,10 +1665,16 @@ bool pkgAcqMetaBase::TransactionHasError()
 {
    for (pkgAcquire::ItemIterator I = Transaction.begin();
         I != Transaction.end(); ++I)
-      if((*I)->Status != pkgAcquire::Item::StatDone &&
-         (*I)->Status != pkgAcquire::Item::StatIdle)
-         return true;
-
+   {
+      switch((*I)->Status) {
+	 case StatDone: break;
+	 case StatIdle: break;
+	 case StatAuthError: return true;
+	 case StatError: return true;
+	 case StatTransientNetworkError: return true;
+	 case StatFetching: break;
+      }
+   }
    return false;
 }
 									/*}}}*/
@@ -1603,24 +1689,7 @@ void pkgAcqMetaBase::CommitTransaction()
    for (std::vector<Item*>::iterator I = Transaction.begin();
         I != Transaction.end(); ++I)
    {
-      if((*I)->PartialFile != "")
-      {
-	 if(_config->FindB("Debug::Acquire::Transaction", false) == true)
-	    std::clog << "mv " << (*I)->PartialFile << " -> "<< (*I)->DestFile << " "
-	       << (*I)->DescURI() << std::endl;
-
-	 Rename((*I)->PartialFile, (*I)->DestFile);
-      } else {
-         if(_config->FindB("Debug::Acquire::Transaction", false) == true)
-            std::clog << "rm "
-                      <<  (*I)->DestFile
-                      << " "
-                      << (*I)->DescURI()
-                      << std::endl;
-         unlink((*I)->DestFile.c_str());
-      }
-      // mark that this transaction is finished
-      (*I)->TransactionManager = 0;
+      (*I)->TransactionState(TransactionCommit);
    }
    Transaction.clear();
 }
@@ -1634,7 +1703,7 @@ void pkgAcqMetaBase::TransactionStageCopy(Item *I,
    I->DestFile = To;
 }
 									/*}}}*/
-// AcqMetaBase::TransactionStageRemoval - Sage a file for removal	/*{{{*/
+// AcqMetaBase::TransactionStageRemoval - Stage a file for removal	/*{{{*/
 void pkgAcqMetaBase::TransactionStageRemoval(Item *I,
                                              const std::string &FinalFile)
 {

apt-pkg/acquire-item.h

@@ -344,7 +344,8 @@ class pkgAcquire::Item : public WeakPointable
       InvalidFormat,
       SignatureError,
       NotClearsigned,
-      MaximumSizeExceeded
+      MaximumSizeExceeded,
+      PDiffError,
    };
 
    /** \brief Rename failed file and set error
@@ -353,6 +354,12 @@ class pkgAcquire::Item : public WeakPointable
     */
    bool RenameOnError(RenameOnErrorState const state);
 
+   enum TransactionStates {
+      TransactionCommit,
+      TransactionAbort,
+   };
+   virtual bool TransactionState(TransactionStates const state);
+
    /** \brief The HashSums of the item is supposed to have than done */
    HashStringList ExpectedHashes;
 
@@ -685,14 +692,12 @@ class APT_HIDDEN pkgAcqDiffIndex : public pkgAcqBaseIndex
     */
    std::string Description;
 
-   /** \brief If the copy step of the packages file is done
-    */
-   bool PackagesFileReadyInPartial;
-
    /** \brief Get the full pathname of the final file for the current URI */
    virtual std::string GetFinalFilename() const;
 
    virtual bool QueueURI(pkgAcquire::ItemDesc &Item);
+
+   virtual bool TransactionState(TransactionStates const state);
  public:
    // Specialized action members
    virtual void Failed(std::string Message,pkgAcquire::MethodConfig *Cnf);
@@ -1010,6 +1015,8 @@ class APT_HIDDEN pkgAcqIndex : public pkgAcqBaseIndex
    /** \brief Get the full pathname of the final file for the current URI */
    virtual std::string GetFinalFilename() const;
 
+   virtual bool TransactionState(TransactionStates const state);
+
    public:
    // Specialized action members
    virtual void Failed(std::string Message,pkgAcquire::MethodConfig *Cnf);

test/integration/framework

@@ -1523,9 +1523,13 @@ aptautotest_aptget_update() {
 	testfilestats "${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt" '%U:%G:%a' '=' "${TEST_DEFAULT_USER}:${TEST_DEFAULT_GROUP}:755"
 	testfilestats "${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists" '%U:%G:%a' '=' "${TEST_DEFAULT_USER}:${TEST_DEFAULT_GROUP}:755"
 	# all copied files are properly chmodded
-	for file in $(find "${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists" -maxdepth 1 -type f ! -name 'lock'); do
+	for file in $(find "${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists" -type f ! -name 'lock'); do
 		testfilestats "$file" '%U:%G:%a' '=' "${TEST_DEFAULT_USER}:${TEST_DEFAULT_GROUP}:644"
 	done
+	if [ "$1" = 'testsuccess' ]; then
+		# failure cases can retain partial files and such
+		testempty find "${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists/partial" -mindepth 1 ! \( -name 'lock' -o -name '*.FAILED' \)
+	fi
 }
 aptautotest_apt_update() { aptautotest_aptget_update "$@"; }
 aptautotest_aptcdrom_add() { aptautotest_aptget_update "$@"; }

test/integration/test-apt-update-expected-size

@@ -34,7 +34,7 @@ test_packagestoobig() {
 	done
 	NEW_SIZE="$(stat --printf=%s aptarchive/dists/unstable/main/binary-i386/Packages)"
 	testfailuremsg "W: Failed to fetch ${1}/dists/unstable/main/binary-i386/Packages  Writing more data than expected ($NEW_SIZE > $SIZE)
-E: Some index files failed to download. They have been ignored, or old ones used instead." aptget update -o Debug::pkgAcquire::Worker=0
+E: Some index files failed to download. They have been ignored, or old ones used instead." aptget update -o Debug::pkgAcquire::Worker=0 -o Debug::Acquire::Transaction=0
 }
 
 methodtest() {

test/integration/test-partial-file-support

@@ -126,18 +126,17 @@ testrun() {
 	testwebserverlaststatuscode '200' "$DOWNLOADLOG"
 }
 
-msgmsg 'http: Test with Content-Length'
-webserverconfig 'aptwebserver::chunked-transfer-encoding' 'false'
-testrun 'http://localhost:8080'
-msgmsg 'http: Test with Transfer-Encoding: chunked'
-webserverconfig 'aptwebserver::chunked-transfer-encoding' 'true'
-testrun 'http://localhost:8080'
+serverconfigs() {
+	msgmsg "${1%%:*}: Test with Content-Length"
+	webserverconfig 'aptwebserver::chunked-transfer-encoding' 'false'
+	testrun "$1"
+	msgmsg "${1%%:*}: Test with Transfer-Encoding: chunked"
+	webserverconfig 'aptwebserver::chunked-transfer-encoding' 'true'
+	testrun "$1"
+}
+
+serverconfigs 'http://localhost:8080'
 
 changetohttpswebserver
 
-msgmsg 'https: Test with Content-Length'
-webserverconfig 'aptwebserver::chunked-transfer-encoding' 'false'
-testrun 'https://localhost:4433'
-msgmsg 'https: Test with Transfer-Encoding: chunked'
-webserverconfig 'aptwebserver::chunked-transfer-encoding' 'true'
-testrun 'https://localhost:4433'
+serverconfigs 'https://localhost:4433'

test/integration/test-pdiff-usage

@@ -47,7 +47,7 @@ testrun() {
 	compressfile 'aptarchive/Packages'
 	generatereleasefiles
 	signreleasefiles
-	rm -rf aptarchive/Packages.diff rootdir/var/lib/apt/lists
+	rm -rf aptarchive/Packages.diff rootdir/var/lib/apt/lists rootdir/var/lib/apt/lists-bak
 	testsuccess aptget update "$@"
 	cp -a rootdir/var/lib/apt/lists rootdir/var/lib/apt/lists-bak
 	testnopackage newstuff