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.

diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc
index 00862fe233614f988e9d1e8d77e17a9e34c510d6..01ce0e6503029c31ef9712d49edc4ee503e0a3b7 100644 (file)
--- a/apt-pkg/acquire-item.cc
+++ b/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)
 {

diff --git a/apt-pkg/acquire-item.h b/apt-pkg/acquire-item.h
index 148728b3c59914b8bd32d41b9a5eaeeb06c34140..33a28671c00595332f93e064f08c38aa4d95b346 100644 (file)
--- a/apt-pkg/acquire-item.h
+++ b/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);

diff --git a/test/integration/framework b/test/integration/framework
index 4229ae1623e96bc1030d4e1c0b387f13efb2aa54..7564a0faff9584835fc45df7641dff05abc78abd 100644 (file)
--- a/test/integration/framework
+++ b/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 "$@"; }

diff --git a/test/integration/test-apt-update-expected-size b/test/integration/test-apt-update-expected-size
index 9f58308aa4eda1a7ff1e8f8ad2a934f422a626c7..7efccaa57494827a6428e26df0609fc8e43e2fde 100755 (executable)
--- a/test/integration/test-apt-update-expected-size
+++ b/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() {

diff --git a/test/integration/test-partial-file-support b/test/integration/test-partial-file-support
index b6b305d25289d41f6b266898263b17862256d107..85046b3eb92616369d45ace64cf5c671ebda0988 100755 (executable)
--- a/test/integration/test-partial-file-support
+++ b/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'

diff --git a/test/integration/test-pdiff-usage b/test/integration/test-pdiff-usage
index 5e759e50e4bc477482a44147a5090a4a856fb239..7d72a6944d2534deb33b5e5dd7416e40f65e5cf6 100755 (executable)
--- a/test/integration/test-pdiff-usage
+++ b/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