[JENKINS-51913] Prevent archiveArtifacts from dumping a stacktrace when empty results are allowed

[gregswift]

See JENKINS-51913.

Testing

Not sure best approach on this, but my guess would be to add an assertion to this test that verifies that the string java.lang.InterruptedException: no matches found within is not in the output?

Proposed changelog entries

Proposed upgrade guidelines

N/A

Submitter checklist

• (If applicable) Jira issue is well described
• Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
  • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
• Appropriate autotests or explanation to why this change has no tests
• For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

Maintainer checklist

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

• There are at least 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 does not block the change
• Changelog entries in the PR title and/or Proposed changelog entries are correct
• Proper changelog labels are set so that the changelog can be generated automatically
• If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
• 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).

[gregswift]

I would generally argue that this stacktrace is always pointless, an error should be fine but 🤷

[MarkEWaite]

Agreed that it needs a test to assert that the stack trace message is not generated when allowEmptyArchive is true.

[jglick]

Seems like it would be safer and better to catch InterruptedException. Not sure what this has to do with allowEmptyArchive in fact.

[gregswift]

@jglick So from my digging it seemed that the main exception the underlying library throws is an intentional for when no artifacts are found. Which is why i took this approach. I do agree that it would introduce a specific situation where if some other exception was thrown, but allow empty was set, then it would hide the exception.

This was just my attempt to get a 2 year old issue that my team ran into moving.. I'm totally open to recommendations. I'm not much of a java dev :D

[jglick]

I think this buglet was introduced by #3265 and it does not really have anything to do with the allowEmptyArchive; it is just that the InterruptedException is “expected”, not a random error like a NullPointerException for which a stack trace would be useful. So it would suffice to write something like (beyond the abilities of GitHub’s PR suggestion feature I am afraid):

try {
    // …
} catch (InterruptedException e) {
    listener.getLogger().println(e.getMessage()); // (in case it is something like "no matches found within 10000")
} catch (Exception e) { // (as before)
    Functions.printStackTrace(e, listener.getLogger());
}

But better would be to improve

jenkins/core/src/main/java/hudson/FilePath.java

Line 2875 in e3b17ef

throw new InterruptedException("no matches found within " + bound);

to throw a dedicated subtype of InterruptedException which ArtifactArchiver could treat specially: the current code will also throw InterruptedException for unrelated reasons that might merit a stack trace, such as an agent connection being closed unexpectedly.

More broadly, anyone encountering this sort of issue is advised to improve their build to either specify a narrower file pattern that does not require as much searching, or to have regular build steps actually move or copy desired files into a dedicated archive directory so that no pattern whatsoever is needed (or just a trivial one like sub/dir/).

[varyvol]

@gregswift did you have the opportunity to review @jglick 's feedback? Also adding a test would be great.

[gregswift]

Sorry.. did review the feedback.. have not gotten a chance to get back to this.

[alecharp]

@gregswift do you know if you will have some bandwidth to work on this in the future? If not, I'd like to add the proposed-for-close label here as there hasn't been any activity for quite some time now.

[gregswift]

ahh damn. sorry. i've changed jobs and totally spaced on this. If you don't hear from me within a week or so go ahead and add the proposed-for-close label.

[basil]

@jglick Given that you have already evaluated the cause of this problem and provided a partial solution in #4913 (comment), might I be able to interest you in giving this kind contributor a carry by completing the original effort? The contributor appears to have moved on, but the patch seems largely viable and just needs some edits. I think it would be a low effort to complete this patch on behalf of the original contributor, and I think the result would be a valuable fix for a years-old annoyance. If, on the other hand, this is asking too much, then feel free to disregard this message and we will likely (tragically) close this pull request due to inactivity in the next week or so.

[basil]

To summarize:

• JENKINS-47142: [JENKINS-47142] Simplifying the logic of ArtifactArchiver.perform, making it friendlier to Pipeline #3265 by Jesse Glick introduced this buglet as acknowledged in [JENKINS-51913] Prevent archiveArtifacts from dumping a stacktrace when empty results are allowed #4913 (comment).
• JENKINS-51913: This PR by Greg Swift was a proposal for a solution, but it needs further refinement. It sounds like we all agree on the next step forward: apply the code review suggestion from Jesse Glick in [JENKINS-51913] Prevent archiveArtifacts from dumping a stacktrace when empty results are allowed #4913 (comment).

We have pinged both Greg Swift and Jesse Glick to see if either of them have an interest in moving forward with the action item from this PR. Since it has been a week since the proposed-for-close label was applied with no positive response (and approximately two years since this PR was filed), I am closing this PR. I have unassigned Greg Swift from JENKINS-51913 and moved the Jira issue back from the In Review state to the Open state.

On behalf of the core team, I would like to thank Greg Swift and Jesse Glick for their contributions. Even though this PR did not make it across the finish line, it was a promising start! I continue to encourage you (or anyone else who is interested!) to pick up this effort and drive it to completion. Users have been annoyed with this behavior for years, and a fix at some point in the future would be highly appreciated! Thanks again.

[gregswift]

@basil I wasn't sure if i'd get to it, but I have just pushed some changes to my branch. It doesn't seem to show here (which is fine). If yall wanna try re-opening it and letting the tests run I can continue verify them and then I will ask for a re-review?

[basil]

The reopen button is grayed out for me with the reason "the no-stacktrace branch was force-pushed or recreated." I think the only path forward is to create a new pull request.

[gregswift]

thats an interesting change to github i haven't experienced before but i can see why its a thing 🤷 so much for cleaning up my work before reopening the PR :D