Store the check expiration info in the metadata

Reviewed-by: ehelin

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java

@@ -62,6 +62,8 @@ class CheckRun {
     private static final String fullNameWarningMarker = "<!-- PullRequestBot full name warning comment -->";
     private final Set<String> newLabels;
 
+    private Duration expiresIn;
+
     private CheckRun(CheckWorkItem workItem, PullRequest pr, Repository localRepo, List<Comment> comments,
                      List<Review> allReviews, List<Review> activeReviews, Set<String> labels,
                      CensusInstance censusInstance, boolean ignoreStaleReviews) throws IOException {
@@ -181,6 +183,13 @@ private List<String> botSpecificChecks(Hash finalHash) throws IOException {
         return ret;
     }
 
+    private void setExpiration(Duration expiresIn) {
+        // Use the shortest expiration
+        if (this.expiresIn == null || this.expiresIn.compareTo(expiresIn) > 0) {
+            this.expiresIn = expiresIn;
+        }
+    }
+
     private Map<String, String> blockingIntegrationLabels() {
         return Map.of("rejected", "The change is currently blocked from integration by a rejection.",
                       "csr", "The change requires a CSR request to be approved.");
@@ -198,16 +207,16 @@ private List<String> botSpecificIntegrationBlockers() {
                     if (iss.isPresent()) {
                         if (!relaxedEquals(iss.get().title(), currentIssue.description())) {
                             var issueString = "[" + iss.get().id() + "](" + iss.get().webUrl() + ")";
-                            ret.add("Title mismatch between PR and JBS for issue " + issueString +
-                                            ExpirationTracker.expiresAfterMarker(Duration.ofMinutes(1)));
+                            ret.add("Title mismatch between PR and JBS for issue " + issueString);
+                            setExpiration(Duration.ofMinutes(1));
                         }
                     } else {
-                        log.warning("Failed to retrieve information on issue " + currentIssue.id() +
-                                            ExpirationTracker.expiresAfterMarker(Duration.ofMinutes(10)));
+                        log.warning("Failed to retrieve information on issue " + currentIssue.id());
+                        setExpiration(Duration.ofMinutes(10));
                     }
                 } catch (RuntimeException e) {
-                    log.warning("Temporary failure when trying to retrieve information on issue " + currentIssue.id() +
-                                        ExpirationTracker.expiresAfterMarker(Duration.ofMinutes(30)));
+                    log.warning("Temporary failure when trying to retrieve information on issue " + currentIssue.id());
+                    setExpiration(Duration.ofMinutes(30));
                 }
             }
         }
@@ -397,7 +406,11 @@ private String getStatusMessage(List<Comment> comments, List<Review> reviews, Pu
             var checks = sourceRepo.allChecks(pr.headHash());
 
             var resultSummary = TestResults.summarize(checks);
-            resultSummary.ifPresent(progressBody::append);
+            if (resultSummary.isPresent()) {
+                progressBody.append(resultSummary.get());
+                var expiration = TestResults.expiresIn(checks);
+                expiration.ifPresent(this::setExpiration);
+            }
         }
 
         if (!integrationBlockers.isEmpty()) {
@@ -437,21 +450,21 @@ private String getStatusMessage(List<Comment> comments, List<Review> reviews, Pu
                             progressBody.append(iss.get().title());
                             if (!relaxedEquals(iss.get().title(), currentIssue.description())) {
                                 progressBody.append(" ⚠️ Title mismatch between PR and JBS.");
+                                setExpiration(Duration.ofMinutes(1));
                             }
-                            progressBody.append(ExpirationTracker.expiresAfterMarker(Duration.ofMinutes(1)));
                             progressBody.append("\n");
                         } else {
                             progressBody.append("⚠️ Failed to retrieve information on issue `");
                             progressBody.append(currentIssue.id());
                             progressBody.append("`.");
-                            progressBody.append(ExpirationTracker.expiresAfterMarker(Duration.ofMinutes(10)));
+                            setExpiration(Duration.ofMinutes(10));
                             progressBody.append("\n");
                         }
                     } catch (RuntimeException e) {
                         progressBody.append("⚠️ Temporary failure when trying to retrieve information on issue `");
                         progressBody.append(currentIssue.id());
                         progressBody.append("`.");
-                        progressBody.append(ExpirationTracker.expiresAfterMarker(Duration.ofMinutes(30)));
+                        setExpiration(Duration.ofMinutes(30));
                         progressBody.append("\n");
                     }
                 }
@@ -892,7 +905,8 @@ private void checkStatus() {
             }
 
             // Calculate current metadata to avoid unnecessary future checks
-            var metadata = workItem.getMetadata(censusInstance, title, updatedBody, pr.comments(), activeReviews, newLabels, pr.isDraft());
+            var metadata = workItem.getMetadata(censusInstance, title, updatedBody, pr.comments(), activeReviews,
+                                                newLabels, pr.isDraft(), expiresIn);
             checkBuilder.metadata(metadata);
         } catch (Exception e) {
             log.throwing("CommitChecker", "checkStatus", e);

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java

@@ -64,13 +64,8 @@ private String encodeReviewer(HostUser reviewer, CensusInstance censusInstance)
     }
 
     String getMetadata(CensusInstance censusInstance, String title, String body, List<Comment> comments,
-                       List<Review> reviews, Set<String> labels, boolean isDraft) {
+                       List<Review> reviews, Set<String> labels, boolean isDraft, Duration expiresIn) {
         try {
-            var hasExpired = ExpirationTracker.hasExpired(body);
-            if (hasExpired) {
-                return String.valueOf(Math.random());
-            }
-
             var approverString = reviews.stream()
                                         .filter(review -> review.verdict() == Review.Verdict.APPROVED)
                                         .map(review -> encodeReviewer(review.reviewer(), censusInstance) + review.hash().hex())
@@ -92,15 +87,19 @@ String getMetadata(CensusInstance censusInstance, String title, String body, Lis
             digest.update(labelString.getBytes(StandardCharsets.UTF_8));
             digest.update(isDraft ? (byte)0 : (byte)1);
 
-            return Base64.getUrlEncoder().encodeToString(digest.digest());
+            var ret = Base64.getUrlEncoder().encodeToString(digest.digest());
+            if (expiresIn != null) {
+                ret += ":" + Instant.now().plus(expiresIn).getEpochSecond();
+            }
+            return ret;
         } catch (NoSuchAlgorithmException e) {
             throw new RuntimeException("Cannot find SHA-256");
         }
     }
 
     private boolean currentCheckValid(CensusInstance censusInstance, List<Comment> comments, List<Review> reviews, Set<String> labels) {
         var hash = pr.headHash();
-        var metadata = getMetadata(censusInstance, pr.title(), pr.body(), comments, reviews, labels, pr.isDraft());
+        var metadata = getMetadata(censusInstance, pr.title(), pr.body(), comments, reviews, labels, pr.isDraft(), null);
         var currentChecks = pr.checks(hash);
 
         if (currentChecks.containsKey("jcheck")) {
@@ -115,15 +114,27 @@ private boolean currentCheckValid(CensusInstance censusInstance, List<Comment> c
                     return true;
                 }
             } else {
-                if (check.metadata().isPresent() && check.metadata().get().equals(metadata)) {
-                    log.finer("No activity since last check, not checking again");
-                    return true;
-                } else {
-                    log.info("PR updated after last check, checking again");
-                    if (check.metadata().isPresent() && (!check.metadata().get().equals(metadata))) {
-                        log.fine("Previous metadata: " + check.metadata().get() + " - current: " + metadata);
+                if (check.metadata().isPresent()) {
+                    var previousMetadata = check.metadata().get();
+                    if (previousMetadata.contains(":")) {
+                        var splitIndex = previousMetadata.lastIndexOf(":");
+                        var stableMetadata = previousMetadata.substring(0, splitIndex);
+                        var expiresAt = Instant.ofEpochSecond(Long.parseLong(previousMetadata.substring(splitIndex + 1)));
+                        if (stableMetadata.equals(metadata) && expiresAt.isAfter(Instant.now())) {
+                            log.finer("Metadata with expiration time is still valid, not checking again");
+                            return true;
+                        }
+                    } else {
+                        if (previousMetadata.equals(metadata)) {
+                            log.finer("No activity since last check, not checking again");
+                            return true;
+                        }
                     }
                 }
+                log.info("PR updated after last check, checking again");
+                if (check.metadata().isPresent() && (!check.metadata().get().equals(metadata))) {
+                    log.fine("Previous metadata: " + check.metadata().get() + " - current: " + metadata);
+                }
             }
         }
 

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBot.java

@@ -132,11 +132,32 @@ private boolean isReady(PullRequest pr) {
         return true;
     }
 
+    private boolean checkHasExpired(PullRequest pr) {
+        var hash = pr.headHash();
+        var currentChecks = pr.checks(hash);
+
+        if (currentChecks.containsKey("jcheck")) {
+            var check = currentChecks.get("jcheck");
+            if (check.metadata().isPresent()) {
+                var metadata = check.metadata().get();
+                if (metadata.contains(":")) {
+                    var expirationString = metadata.substring(metadata.lastIndexOf(":") + 1);
+                    var expiresAt = Instant.ofEpochSecond(Long.parseLong(expirationString));
+                    if (expiresAt.isBefore(Instant.now())) {
+                        log.info("Check metadata has expired (expired at: " + expiresAt + ")");
+                        return true;
+                    }
+                }
+            }
+        }
+        return false;
+    }
+
     private List<WorkItem> getWorkItems(List<PullRequest> pullRequests) {
         var ret = new LinkedList<WorkItem>();
 
         for (var pr : pullRequests) {
-            if (ExpirationTracker.hasExpired(pr.body()) || updateCache.needsUpdate(pr, Duration.ofMinutes(5))) {
+            if (updateCache.needsUpdate(pr, Duration.ofMinutes(5)) || checkHasExpired(pr)) {
                 if (!isReady(pr)) {
                     continue;
                 }

bots/pr/src/main/java/org/openjdk/skara/bots/pr/TestResults.java

@@ -62,27 +62,32 @@ private static boolean ignoredCheck(String checkName) {
         return lcName.contains("jcheck") || lcName.contains("prerequisites") || lcName.contains("post-process");
     }
 
-    static Optional<String> summarize(List<Check> checks) {
-        // Retain only the latest when there are multiple checks with the same name
+    // Retain only the latest when there are multiple checks with the same name
+    private static Collection<Check> latestChecks(List<Check> checks) {
         var latestChecks = checks.stream()
                                  .filter(check -> !ignoredCheck(check.name()))
                                  .sorted(Comparator.comparing(Check::startedAt, ZonedDateTime::compareTo))
                                  .collect(Collectors.toMap(Check::name, Function.identity(), (a, b) -> b, LinkedHashMap::new));
+        return latestChecks.values();
+    }
+
+    static Optional<String> summarize(List<Check> checks) {
+        var latestChecks = latestChecks(checks);
         if (latestChecks.isEmpty()) {
             return Optional.empty();
         }
 
-        var platforms = latestChecks.values().stream()
+        var platforms = latestChecks.stream()
                                     .map(check -> platformFromName(check.name()))
                                     .collect(Collectors.toCollection(TreeSet::new));
-        var flavors = latestChecks.values().stream()
+        var flavors = latestChecks.stream()
                                   .map(check -> flavorFromName(check.name()))
                                   .collect(Collectors.toCollection(TreeSet::new));
         if (platforms.isEmpty() || flavors.isEmpty()) {
             return Optional.empty();
         }
 
-        var platformFlavors = latestChecks.values().stream()
+        var platformFlavors = latestChecks.stream()
                                           .collect(Collectors.groupingBy(check -> platformFromName(check.name()))).entrySet().stream()
                                           .collect(Collectors.toMap(Map.Entry::getKey,
                                                                     entry -> entry.getValue().stream()
@@ -134,7 +139,7 @@ static Optional<String> summarize(List<Check> checks) {
             }
         }
 
-        var failedChecks = latestChecks.values().stream()
+        var failedChecks = latestChecks.stream()
                                        .filter(check -> check.status() == CheckStatus.FAILURE)
                                        .sorted(Comparator.comparing(Check::name))
                                        .collect(Collectors.toList());
@@ -160,14 +165,18 @@ static Optional<String> summarize(List<Check> checks) {
             }
         }
 
-        var needRefresh = latestChecks.values().stream()
-                .filter(check -> check.status() == CheckStatus.IN_PROGRESS)
-                .findAny();
+        return Optional.of(resultsBody.toString());
+    }
+
+    static Optional<Duration> expiresIn(List<Check> checks) {
+        var latestChecks = latestChecks(checks);
+        var needRefresh = latestChecks.stream()
+                                      .filter(check -> check.status() == CheckStatus.IN_PROGRESS)
+                                      .findAny();
         if (needRefresh.isPresent()) {
-            resultsBody.append("\n");
-            resultsBody.append(ExpirationTracker.expiresAfterMarker(Duration.ofSeconds(30)));
+            return Optional.of(Duration.ofSeconds(30));
+        } else {
+            return Optional.empty();
         }
-
-        return Optional.of(resultsBody.toString());
     }
 }