Address ConcurrentModificationException which occasionally happens during XStream saving of running workflows [JENKINS-76294]

[jimklimov]

See https://issues.jenkins.io/browse/JENKINS-76294, #336 and jenkinsci/lockable-resources-plugin#825 (for example of similar problem and fix).

This PR adds @GuardedBy and synchronized protection for Map SafeArchiveServingAction.fileChecksums which is implicated in job crashes:

java.util.ConcurrentModificationException
	at java.util.HashMap$HashIterator.nextNode(HashMap.java:1445)
...
Caused: java.lang.RuntimeException: Failed to serialize net.masterthought.jenkins.SafeArchiveServingAction#fileChecksums
  for class net.masterthought.jenkins.SafeArchiveServingRunAction
...

and those crashes are likely due to the plugin sometimes changing the map while another thread in the Jenkins controller says it should be serialized (e.g. to save workflow state). To address the latter, a writeReplace() method is introduced (which is found by XStreams serialization via reflection, along with the properties it wants to save), and constructs a synchronized copy of the current object to be actually saved in its stead.

Testing done

OBSOLETED: So far none. There are no existing unit tests in this plugin source code, and I am not sure where to start from scratch. An adaptation of test added for Lockable Resources (see above), trying to run many parallel builds and stages in them, while saving the workflows and server state internally and externally, would be a likely start.

UPDATED: See also PR #537 which adds the test to original code base and exposes the bug. Same commit is now added to this PR branch to check that the issue is no longer reproducible (passed on my system).

Note that previously this repository had no tests, so relevant pom.xml bits were copied from Lockable Resource plugin (along with the convoluted test to force the race condition and crash the best we can). The pom.xml changes in particular may be a bit of overkill for this one test case, pulling in components that are not really referenced (I did not investigate deeper). They might be useful if someone adds other tests later though.

Submitter checklist

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests that demonstrate the feature works or the issue is fixed

[jimklimov]

Unit test survived, proposed fix is working.

[jimklimov]

Note for reviewers: I did not check if de-serialization works correctly (and not sure how to do so), but with a full-object replica with no prop-renaming trickery, there is little reason why it would not, right? :)

[damianszczepanik]

Class which you have been updated for this problem was taken from https://github.com/jenkinsci/htmlpublisher-plugin - did you check if and how this problem was fixed there?

[jimklimov]

@damianszczepanik : can't quickly say which of those sources was the origin of yours :) but...

• https://github.com/jenkinsci/htmlpublisher-plugin/blob/master/src/main/java/htmlpublisher/util/FileEntryQueue.java - uses concurrency-friendly primitives (the suggestions in ChatGPT, linked from JIRA ticket, mentioned those as one of possible solutions - but still pushed for persisting an immutable copy; I am not much of a Java dev so can't elaborate much more on the merits, pros and cons of specific offered solutions):

import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.LinkedBlockingQueue;

• https://github.com/jenkinsci/htmlpublisher-plugin/blob/master/src/main/java/htmlpublisher/HtmlPublisher.java - private final List<HtmlPublisherTarget> reportTargets; seems to be only set once, in the constructor
• upon cursory review, other class properties are not of complex types so should not be seen half-changed in a race condition

[damianszczepanik]

what is the test result if this timeout is reached?

[damianszczepanik]

do we need to throw an Exception here, suggest to put something concrete if possible

[damianszczepanik]

I don't like this approach of merging two definition into single line

[damianszczepanik]

can you put this java code into separate java file and load as a resource into the string? it help reading such code as putting this into java file enables syntax and coloring