Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-47748

Second archiveArtifacts step fails silently with compress-artifacts-plugin

    • Icon: Bug Bug
    • Resolution: Unresolved
    • Icon: Minor Minor
    • compress-artifacts-plugin 1.10
      Jenkins 2.73.2
      Windows Server 2012 R2

      I put two archiveArtifacts steps in a declarative Jenkinsfile but only the files archived by the first step show up in the Jenkins UI.

      dir('bin/Release') {
          archiveArtifacts artifacts: 'data/*.xml', fingerprint: true, onlyIfSuccessful: true
          archiveArtifacts artifacts: '**/*', excludes: '**/*.xml', fingerprint: true, onlyIfSuccessful: true
      }
      

      The build directory in the Jenkins master then contains both "archive.zip" with files from the first step, and "archive.zip.writing.zip" with files from the second step. I think the tempArchive.renameTo(archive); call in ZipStorage failed. There is a comment "TODO support updating entries", so this is apparently a known problem.

      However, if this kind of thing is not supported, then I think the build should at least have failed with an explanation in Console Output. I got no warnings there, and the build succeeded:

      [Pipeline] dir
      Running in CENSORED\bin\Release
      [Pipeline] {
      [Pipeline] archiveArtifacts
      Archiving artifacts
      Recording fingerprints
      [Pipeline] archiveArtifacts
      Archiving artifacts
      Recording fingerprints
      [Pipeline] }
      [Pipeline] // dir
      [Pipeline] }
      

          [JENKINS-47748] Second archiveArtifacts step fails silently with compress-artifacts-plugin

          The error-handling flaw seems distinct from JENKINS-45200, in which the job ultimately failed. Here, the job succeeded but some of the artifacts were missing.

          Kalle Niemitalo added a comment - The error-handling flaw seems distinct from JENKINS-45200 , in which the job ultimately failed. Here, the job succeeded but some of the artifacts were missing.

          The tempArchive.renameTo(archive) call goes to File.renameTo(File), which returns false if it fails, and compress-artifacts-plugin ignores this return value. So, that's why the run succeeded despite the error.

          I didn't find any other places where the plugin ignores return values from File.

          Kalle Niemitalo added a comment - The tempArchive.renameTo(archive) call goes to File.renameTo(File) , which returns false if it fails, and compress-artifacts-plugin ignores this return value. So, that's why the run succeeded despite the error. I didn't find any other places where the plugin ignores return values from File.

          Lubo Varga added a comment -

          I am using scripted pipeline, where there are multiple parallel build steps (owasp dependency check, mvn test, code coverage) and they could run on different nodes/dockers, so I need to call archiveArtifacts multiple times.

          Is there any methodical workaround for situations like these?

          Lubo Varga added a comment - I am using scripted pipeline, where there are multiple parallel build steps (owasp dependency check, mvn test, code coverage) and they could run on different nodes/dockers, so I need to call archiveArtifacts multiple times. Is there any methodical workaround for situations like these?

          This is just not implemented in compress-artifacts plugin. It should not be that difficult as underlying truezip library appears to support that.

          Help wanted!

          Oliver Gondža added a comment - This is just not implemented in compress-artifacts plugin. It should not be that difficult as underlying truezip library appears to support that. Help wanted!

          luvarqpp, a workaround might be to stash the files from each node and unstash all of them to the same node, then archive them from there using only one archiveArtifacts step.

          Kalle Niemitalo added a comment - luvarqpp , a workaround might be to stash the files from each node and unstash all of them to the same node, then archive them from there using only one archiveArtifacts step.

          I finally tried the workaround. It works, but it is quite inconvenient with matrix because you need to give each stash a unique name and later unstash them with the same names. matrix in Declarative Pipeline doesn't support taking the axis values from a variable, so you cannot define the list of values just once and then use that both in matrix and in a loop that runs unstash.

          Kalle Niemitalo added a comment - I finally tried the workaround. It works, but it is quite inconvenient with matrix because you need to give each stash a unique name and later unstash them with the same names. matrix in Declarative Pipeline doesn't support taking the axis values from a variable, so you cannot define the list of values just once and then use that both in matrix and in a loop that runs unstash .

          Thinking about how to fix this. There might be a problem if multiple agents in a parallel build run archiveArtifacts steps at the same time, and multiple threads in the Jenkins controller then attempt to update the same zip file. I suppose I should look at how the similar problem has been solved in the fingerprint storage.

          Kalle Niemitalo added a comment - Thinking about how to fix this. There might be a problem if multiple agents in a parallel build run archiveArtifacts steps at the same time, and multiple threads in the Jenkins controller then attempt to update the same zip file. I suppose I should look at how the similar problem has been solved in the fingerprint storage.

          Kalle Niemitalo added a comment - - edited

          FileFingerprintStorage synchronizes on the Fingerprint instance and then uses AtomicFileWriter to rewrite the file. The Fingerprint instance corresponds to a single file and a single hash value, and FingerprintMap tries not to create two Fingerprint instances for the same hash value. More in JENKINS-67602 (comment).

          AtomicFileWriter might be too slow for zipped artifacts; if there already is a zip file with hundreds of megabytes, I don't think the plugin should copy all of it every time a parallel stage runs the archiveArtifacts step to add another artifact.

          Instead, the plugin could either

          • Update the zip file in place. This risks corrupting the file if the Jenkins controller is killed during the update, but then one can just restart the build. This probably needs some kind of synchronization for parallel builds. Perhaps it can synchronize on the CompressingArtifactManager instance, of which there should not be more than one for the same Build, or on a ReadWriteLock referenced by a transient field of CompressingArtifactManager.
          • Or, create a separate zip file for each archiveArtifacts step (i.e. each ArtifactManager.archive call), then merge them at the end of the build. This would complicate ZipStorage, which would then have to merge the file lists in memory, if someone looks at the artifacts before the build finishes.

          Kalle Niemitalo added a comment - - edited FileFingerprintStorage synchronizes on the Fingerprint instance and then uses AtomicFileWriter to rewrite the file. The Fingerprint instance corresponds to a single file and a single hash value, and FingerprintMap tries not to create two Fingerprint instances for the same hash value. More in JENKINS-67602 (comment) . AtomicFileWriter might be too slow for zipped artifacts; if there already is a zip file with hundreds of megabytes, I don't think the plugin should copy all of it every time a parallel stage runs the archiveArtifacts step to add another artifact. Instead, the plugin could either Update the zip file in place. This risks corrupting the file if the Jenkins controller is killed during the update, but then one can just restart the build. This probably needs some kind of synchronization for parallel builds. Perhaps it can synchronize on the CompressingArtifactManager instance, of which there should not be more than one for the same Build, or on a ReadWriteLock referenced by a transient field of CompressingArtifactManager. Or, create a separate zip file for each archiveArtifacts step (i.e. each ArtifactManager.archive call), then merge them at the end of the build. This would complicate ZipStorage , which would then have to merge the file lists in memory, if someone looks at the artifacts before the build finishes.

          One hacky workaround would be to define a JobProperty that disables compression of artifacts, and make CompressingArtifactManagerFactory.managerFor(Run<?,?> build) return null if that exists on the Job. Then developers could disable compression of artifacts on individual jobs, where parallel builds are used or the artifacts are known not to be compressible. That would perhaps be more complex to implement than updating the zip file in place.

          Kalle Niemitalo added a comment - One hacky workaround would be to define a JobProperty that disables compression of artifacts, and make CompressingArtifactManagerFactory.managerFor(Run<?,?> build) return null if that exists on the Job. Then developers could disable compression of artifacts on individual jobs, where parallel builds are used or the artifacts are known not to be compressible. That would perhaps be more complex to implement than updating the zip file in place.

          For updating in place, ZipStorage.open() would have to lock the read lock, and ZipStorage.EntryInputStream.close() would have to unlock the read lock. If the ZipStorage.EntryInputStream is leaked, this could cause the read lock to be left locked for an excessive time and block any threads that attempt to archive more artifacts of that build. ArtifactManager.archive is fortunately declared as throwing InterruptedException, so a user could at least abort the build and interrupt the blocked threads.

          I wonder if the future version of ZipStorage.archive should perhaps use Lock.tryLock(long time, TimeUnit unit) to limit the duration of the wait, and if the write lock times out, then take a read lock, copy the zip file instead of updating it in place, and switch to a new ReadWriteLock. There would have to be a separate lock to prevent two threads from replacing the ReadWriteLock in parallel, and the implementation would be difficult to test. I suppose it is better to fix any ZipStorage.EntryInputStream leaks, instead.

          Kalle Niemitalo added a comment - For updating in place, ZipStorage.open() would have to lock the read lock, and ZipStorage.EntryInputStream.close() would have to unlock the read lock. If the ZipStorage.EntryInputStream is leaked, this could cause the read lock to be left locked for an excessive time and block any threads that attempt to archive more artifacts of that build. ArtifactManager.archive is fortunately declared as throwing InterruptedException, so a user could at least abort the build and interrupt the blocked threads. I wonder if the future version of ZipStorage.archive should perhaps use Lock.tryLock(long time, TimeUnit unit) to limit the duration of the wait, and if the write lock times out, then take a read lock, copy the zip file instead of updating it in place, and switch to a new ReadWriteLock. There would have to be a separate lock to prevent two threads from replacing the ReadWriteLock in parallel, and the implementation would be difficult to test. I suppose it is better to fix any ZipStorage.EntryInputStream leaks, instead.

            Unassigned Unassigned
            kon Kalle Niemitalo
            Votes:
            3 Vote for this issue
            Watchers:
            7 Start watching this issue

              Created:
              Updated: