PeepholePermalink.Cache

[jglick]

Jenkins persistently caches permalink values such as lastUnstableBuild to a text file $JENKINS_HOME/job/$name/builds/permalinks, with the format

lastUnstableBuild 123
lastFailedBuild -1

where -1 indicates a negative cache. This was implemented because lazy loading of builds means we wish to avoid loading too many historical build records into memory. If you are so lucky as to have had thousands of builds without a single failure, without a cache the controller would need to search them all after every startup just to find that there has never been an unstable build. So the cache is read into memory at most once per job, and written whenever some permalink changes (typically after a build completes).

In CloudBees CI running in high availability mode this cache implementation was problematic and needed to be replaced with something appropriate for concurrent access. Originally I had tried invalidating the in-memory cache and letting it be reread from disk, but this was a bit inefficient and still vulnerable to race conditions. A more satisfactory solution is to extract the singleton implementation into a beta extension point, similarly to #9846.

The behavior for Jenkins should be identical, but there may be an ancillary benefit that (in my opinion) the refactored logic is easier to read and reason about. In the original code, an int variable was overloaded to mean a build number (when >0), a negative cache result when -1, or no cache result when 0. There actually appears to have been a mistake in the logic (albeit a harmless one) caused by treating 0 as if it were a legitimate build number. This seemed like a great opportunity to practice using sealed types as unions to catch such mistakes in the compiler. Additionally, all code relating to the file format (and the rather arbitrary choice of -1 to mean a negative cache, rather than say - or null) is encapsulated in the default cache implementation.

Testing done

The existing PeepholePermalinkTest (in the test module) should cover the existing logic. The viability of an alternate extension point implementation is covered by a test in CloudBees CI.

Proposed changelog entries

• N/A

Proposed upgrade guidelines

N/A

Desired reviewers

Before the changes are marked as ready-for-merge:

Maintainer checklist

Preview Give feedback

1. There are at least two (2) approvals for the pull request and no outstanding requests for change.
2. Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
3. Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
4. Proper changelog labels are set so that the changelog can be generated automatically.
5. If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
6. If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

[jglick]

getNearestBuild is not used by the code touched here, but I noticed that its Javadoc was simply wrong: it finds the oldest build newer than a given point, not the youngest build!

Also the comparison was phrased in the opposite direction of what you might intuitively expect.

[jglick]

Rather than repeating the terminology used in the method name, trying to clarify.

[jglick]

Arguably clearer phrasing. Encountered while studying the behavior of getNearestOldBuild(0) (see elsewhere).

[jglick]

The original implementation in fact called getNearestOldBuild(0), which unconditionally returns null. You can deduce this from the original Javadoc but the default implementation (overridden in LazyBuildMixIn) based on TailMap was potentially confusing, since the map in question is sorted in reverse order. I noticed there was no direct test coverage of these methods and decided to add some to clarify the meaning.

[jglick]

It took me a while to realize that this was sometimes called with n == 0 in which case the return value of getNearestOldBuild was unconditionally null, so the clause had no effect (b == null before and after). Now this method is called only when processing Some, with a positive argument.

[jglick]

The Git diff unfortunately fails to associate this hunk with the original method call. I just converted the body into a while loop for clarity.

[jglick]

Note that Map.getOrDefault cannot be used because it is not typed to permit a return value wider than the map value type.

[jglick]

Note that this can be written more clearly in Java 21:

--- core/src/main/java/jenkins/model/PeepholePermalink.java
+++ core/src/main/java/jenkins/model/PeepholePermalink.java
@@ -248,7 +248,10 @@ public abstract class PeepholePermalink extends Permalink implements Predicate<R
                         for (var entry : cache.entrySet()) {
                             cw.write(entry.getKey());
                             cw.write(' ');
-                            cw.write(Integer.toString(entry.getValue() instanceof Cache.Some some ? some.number : -1));
+                            cw.write(Integer.toString(switch (entry.getValue()) {
+                                case Some some -> some.number;
+                                case None none -> -1;
+                            }));
                             cw.write('\n');
                         }
                         cw.commit();

[jglick]

(broken out of the original resolve method; the rest is mostly now in Some)

[timja]

/label ready-for-merge

---

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!