[JENKINS-75602] Report URL in update center failure messages

[adhamahmad]

Fixes #16723

Testing done

Tested the change manually by executing the affected functionality locally.
Attached a screenshot showing the expected behavior after applying the change.

Proposed changelog entries

• Report URL in update center failure messages.

Proposed changelog category

/label bug

Proposed upgrade guidelines

N/A

Submitter checklist

• The issue, if it exists, is well-described.
• The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
• There is automated testing or an explanation as to why this change has no tests.
• New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
• New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
• UI changes do not introduce regressions when enforcing the current default rules of Content Security Policy Plugin. In particular, new or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
• For dependency updates, there are links to external changelogs and, if possible, full differentials.
• For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@mention

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

Maintainer checklist

• There are at least two (2) approvals for the pull request and no outstanding requests for change.
• Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
• Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
• Proper changelog labels are set so that the changelog can be generated automatically.
• 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).
• If it would make sense to backport the change to LTS, be a Bug or Improvement, and either the issue or pull request must be labeled as lts-candidate to be considered.

[MarkEWaite]

I don't understand how this change resolves the reported issue:

• [JENKINS-75602] Error handling: hudson.PluginManager.checkUpdatesServer: java.net.SocketException: Network is unreachable #16723

The stack trace in that message (from Jenkins 2.492.3) is (with some sections removed for brevity):

Apr 24 14:47:57 thor jenkins[5535]: java.net.SocketException: Network is unreachable
Apr 24 14:47:57 thor jenkins[5535]: at java.base/sun.nio.ch.Net.connect0(Native Method)
... (deleted for brevity)
Apr 24 14:47:57 thor jenkins[5535]: at hudson.model.DownloadService.loadJSON(DownloadService.java:122)
Apr 24 14:47:57 thor jenkins[5535]: at hudson.model.UpdateSite.updateDirectlyNow(UpdateSite.java:217)
Apr 24 14:47:57 thor jenkins[5535]: at hudson.model.UpdateSite.updateDirectlyNow(UpdateSite.java:212)
Apr 24 14:47:57 thor jenkins[5535]: at hudson.PluginManager.checkUpdatesServer(PluginManager.java:2168)
... (deleted for brevity)

Line 2168 in 2.492.3 PluginManager.java is before your change. It looks like this:

            FormValidation v = site.updateDirectlyNow();

Since SocketException is an exception, the new code that you added will not be called because the exception will be thrown before your new code is reached.

The stack trace indicates that the exception appeared during a daily check for updates. If that's the case, then I think that the best place to insert the additional error message is in the last call in the Jenkins code base before it enters Java libraries. That would be:

core/src/main/java/hudson/model/DownloadService.java where the diff would look like this:

     @Restricted(NoExternalUse.class)
     public static String loadJSON(URL src) throws IOException {
         URLConnection con = ProxyConfiguration.open(src);
         if (con instanceof HttpURLConnection) {
             // prevent problems from misbehaving plugins disabling redirects by default
             ((HttpURLConnection) con).setInstanceFollowRedirects(true);
         }
         try (InputStream is = con.getInputStream()) {
             String jsonp = IOUtils.toString(is, StandardCharsets.UTF_8);
             int start = jsonp.indexOf('{');
             int end = jsonp.lastIndexOf('}');
             if (start >= 0 && end > start) {
                 return jsonp.substring(start, end + 1);
             } else {
                 throw new IOException("Could not find JSON in " + src);
             }
+        } catch (IOException ioe) {
+            throw new IOException("Failed to load JSON from " + src, ioe);
         }
     }

That corresponds to the stack trace in the bug report and preserves the original IOException.

[MarkEWaite]

As far as I can tell from reading the stack trace in the original report, the changes in the PluginManager class won't resolve the issue that was reported.

[MarkEWaite]

@adhamahmad could you review the changes that I proposed in 8e91388? I was interested enough in your pull request that I spent some time exploring it and did not want to lose the results of that exploration.

[MarkEWaite]

The automated tests of the plugin manager fail nicely and show that my change makes the experience worse for the Jenkins administrator who makes a mistake from the "Advanced Settings" page of the plugin manager. Various forms of invalid update center URLs are illustrated here as "before" and "after".

In each of the cases, the unmodified code presents a clearer message to the administrator when they make a mistake in the plugin manager "Advanced Settings" page. The existing code provides a message that is easier to understand.

Bad hostname in update center URL

Before

After

File not found in update center URL

Before

After

Empty file downloaded from update center URL

Before

After

[MarkEWaite]

@adhamahmad I won't have more time to spend on this for at least a week. I think that we need some way to retain the desirable user experience of the current code while still providing more details when a failure is logged. If you want to explore it further, that would be great with me.

[adhamahmad]

@MarkEWaite Thanks for taking the time to explore this. I’m reviewing the changes now, including the impact on the user-facing messages versus logging, and will follow up with feedback or updates shortly.

[zbynek]

It's also usually helpful to avoid throwing exceptions inside of a catch block. In this case that should also help with adding the URL to the message twice.

Untested patch

Index: core/src/main/java/hudson/model/DownloadService.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/core/src/main/java/hudson/model/DownloadService.java b/core/src/main/java/hudson/model/DownloadService.java
--- a/core/src/main/java/hudson/model/DownloadService.java	(revision 648ad341564a6df64e97b6d2c99ddaf4c905ad52)
+++ b/core/src/main/java/hudson/model/DownloadService.java	(date 1765837735782)
@@ -44,6 +44,7 @@
 import java.io.InputStream;
 import java.lang.reflect.Field;
 import java.net.HttpURLConnection;
+import java.net.SocketException;
 import java.net.URL;
 import java.net.URLConnection;
 import java.net.URLEncoder;
@@ -114,12 +115,7 @@
      */
     @Restricted(NoExternalUse.class)
     public static String loadJSON(URL src) throws IOException {
-        URLConnection con = ProxyConfiguration.open(src);
-        if (con instanceof HttpURLConnection) {
-            // prevent problems from misbehaving plugins disabling redirects by default
-            ((HttpURLConnection) con).setInstanceFollowRedirects(true);
-        }
-        try (InputStream is = con.getInputStream()) {
+        try (InputStream is = openConnection(src)) {
             String jsonp = IOUtils.toString(is, StandardCharsets.UTF_8);
             int start = jsonp.indexOf('{');
             int end = jsonp.lastIndexOf('}');
@@ -131,6 +127,19 @@
         }
     }
 
+    private static InputStream openConnection(URL src) throws IOException {
+        URLConnection con = ProxyConfiguration.open(src);
+        if (con instanceof HttpURLConnection) {
+            // prevent problems from misbehaving plugins disabling redirects by default
+            ((HttpURLConnection) con).setInstanceFollowRedirects(true);
+        }
+        try {
+            return con.getInputStream();
+        } catch (SocketException ex) {
+            throw new IOException("Cannot fetch data from " + src  + " due to " + ex, ex);
+        }
+    }
+
     /**
      * Loads JSON from a JSON-with-{@code postMessage} URL.
      * @param src a URL to a JSON HTML file (typically including {@code id} and {@code version} query parameters)
@@ -139,12 +148,7 @@
      */
     @Restricted(NoExternalUse.class)
     public static String loadJSONHTML(URL src) throws IOException {
-        URLConnection con = ProxyConfiguration.open(src);
-        if (con instanceof HttpURLConnection) {
-            // prevent problems from misbehaving plugins disabling redirects by default
-            ((HttpURLConnection) con).setInstanceFollowRedirects(true);
-        }
-        try (InputStream is = con.getInputStream()) {
+        try (InputStream is = openConnection(src)) {
             String jsonp = IOUtils.toString(is, StandardCharsets.UTF_8);
             String preamble = "window.parent.postMessage(JSON.stringify(";
             int start = jsonp.indexOf(preamble);

[adhamahmad]

Hey @MarkEWaite @zbynek,

Asking for guidance: can we include the URL in logging only while rethrowing the original exception to maintain the current UI behavior (similar to my previous commit 96cc0e5) ?

For reference, here’s the UI error message in case of bad hostname after implementing the suggested openConnection() method:

[adhamahmad]

@MarkEWaite
I think the safest approach is to log the update site URL only, while rethrowing the original exception so the current UI behavior is preserved.
Does that align with your expectations, or would you prefer a different direction?