[JENKINS-54126] if strange 404 when cache is on by rsandell · Pull Request #204 · jenkinsci/github-branch-source-plugin

rsandell · 2019-02-12T15:51:38Z

temporarily turn it off and try again

…ff and try again

stephenc · 2019-02-12T15:59:46Z

+                    //JENKINS-54126 try again without cache headers
+                    final Connector.ForceValidationOkHttpConnector oldConnector = (Connector.ForceValidationOkHttpConnector) gitHub.getConnector();
+                    try {
+                        //TODO I'm not sure we are alone in using this connector so maybe concurrent modification problems


maybe have a withoutCache method that takes a closure, that way you could use a lock in the connector to ensure the correct one gets used... if you are worried about concurrent requests

stephenc

I think we're going to have to accept this kind of hacks until we find the root cause.

May want to have a system property to turn off if cutting a release

stephenc · 2019-02-12T16:01:48Z

                }
                // means that does not exist and this is handled below this try/catch block.
            }
+            if (finicky) {


if (finicky && !Boolean.getBoolean("disable hack")) { (or better an effectively final field) so that if your finicky hack is causing issues in production you can at least turn it off without restarting

rsandell · 2019-02-13T11:22:58Z

I've been staring at that spotcheck error and it doesn't make sense to me, it seems like something obvious but I can't see it.

stephenc · 2019-02-13T12:31:35Z

                // means that does not exist and this is handled below this try/catch block.
            }
+            if (finicky) {
+                final Optional<List<String>> status = Optional.ofNullable(fnf.getResponseHeaderFields() != null ? fnf.getResponseHeaderFields().get(null) : null);


I think you are not supposed to pass an explicit null to Optional.ofNullable... so try

fnf.getResponseHeaderFields() != null ? Optional.ofNullable(fnf.getResponseHeaderFields().get(null)) : Optional.empty())

Or perhaps it is the .get(null) that it objects to @rsandell

kshultzCB · 2019-02-13T13:45:58Z

To help set context for the strange looking 404 errors, a little back story on the tests I've been running.

The relationship between strange 404s and missing branches/PRs

I've got a repository that I'm using for this work. It's nothing fancy, but does hold the shell scripts I'm using to automatically create / delete PR branches.
On the Jenkins master, I've got a custom build of okhttp-urlconnection, placed in plugins/github/WEB-INF/lib/ and /github-api/WEB-INF/lib/. Through this change, I can see additional log output.
When running the create-pull-requests.sh script, I'm tail ing the Jenkins log, which is at the default location of /var/log/jenkins/jenkins.log.
Occasionally, but not always, GitHub returns a 404 during these branch create operations. A complete log of "create 10 PRs" can be seen in this sample log, but the 404 I'm talking about specifically looks like this:

HTTP Headers:

Server: GitHub.com,
Date: Fri, 08 Feb 2019 23:13:28 GMT,
Content-Type: application/json; charset=utf-8,
Transfer-Encoding: chunked,
Status: 404 Not Found,
X-RateLimit-Limit: 5000,
X-RateLimit-Remaining: 4998,
X-RateLimit-Reset: 1549671208,
X-OAuth-Scopes: repo,
X-Accepted-OAuth-Scopes: repo,
X-GitHub-Media-Type: github.v3; format=json,
Access-Control-Expose-Headers: ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type,
Access-Control-Allow-Origin: *,
Strict-Transport-Security: max-age=31536000; includeSubdomains; preload,
X-Frame-Options: deny,
X-Content-Type-Options: nosniff,
X-XSS-Protection: 1; mode=block,
Referrer-Policy: origin-when-cross-origin, strict-origin-when-cross-origin,
Content-Security-Policy: default-src 'none',
Content-Encoding: gzip,
X-GitHub-Request-Id: E898:4177:15A16C5:2B3EC57:5C5E0D18,
OkHttp-Sent-Millis: 1549667608356,
OkHttp-Received-Millis: 1549667608397,
Server returned http error code: 404 and error message: �1�0
                                                               Ы ��f`��
�WɉClwA�X�{S�j*���b��x�t�,�7tKV�?|��w��k�X�R���\6i�����R��
                                                                #�-L�j���O�An

Only when this 404 was present during a branch create operation did I see inconsistent behavior from Jenkins. But, importantly, I have seen a direct relationship between the 404s and the inconsistent Jenkins behavior. If GitHub does not report a 404, everything works fine. But if it does, the inconsistent branches and PRs would appear in Jenkins, every time.

That inconsistent behavior looks like:

Branches being shown on the Branches tab which should not be there, since we're supposed to be Excluding branches which are also filed as PRs:
Missing PRs. In this screenshot, PR-10, PR-11, and PR-12 are missing:

How are things going with this PR in place

After creating several hundred PRs, including a duplicate repository intended to speed up the work - I did not see a 404 all day yesterday from GitHub during creation of a PR. Not a single time. Which is...frustrating.
As a control, I switched back to the original beta github-branch-source plugin I'd been using previously. Still no 404s from GitHub.
I reliably get 404s from GitHub when performing branch deletes, via this script. But, from my understanding, orphaned items like this will only be "cleaned up" with a full scan - a webhook will not trigger it. So, the branches and PRs remain in place on Jenkins until I do that, at which point, they are removed as expected.

bitwiseman · 2019-02-14T00:57:02Z

                // means that does not exist and this is handled below this try/catch block.
            }
+            if (finicky) {
+                final Optional<List<String>> status = Optional.ofNullable(fnf.getResponseHeaderFields() != null ? fnf.getResponseHeaderFields().get(null) : null);


Suggested change

final Optional<List<String>> status = Optional.ofNullable(fnf.getResponseHeaderFields() != null ? fnf.getResponseHeaderFields().get(null) : null);

final Optional<List<String>> status = Optional.ofNullable(fnf.getResponseHeaderFields() != null ? Optional.ofNullable(fnf.getResponseHeaderFields().get(null)) : Optional.empty());

Turning @stephenc 's comment into a suggestion.

That didn't help. It was because it thought that just because the method didn't return null on the first call it could on the second call 🤦‍♂️ .

bitwiseman · 2019-02-14T01:12:19Z

@rsandell - With the cache turned on Jenkins fetches the repo and then get that specific Jenkinsfile at a particular revision, right?

@kshultzCB - So what you're saying is we don't have a stable recreate of the issue using the unmodified plugin?

kshultzCB · 2019-02-14T01:35:33Z

@bitwiseman - I'm saying "on Monday, we had a fairly reliable recreate. And now, I got nothin'." :( Not a single recreate with the unmodified plugin since Tuesday afternoon, and I've done it hundreds of times.

I did reach out to GitHub themselves about the issue. They pointed me to a couple of Stack Overflow articles (1, 2), both of which talk about okhttp being at fault for the gibberish characters.

I got curious and started poking around. The version of okhttp that's in use by github-plugin and github-api-plugin is 2.7.5, which looks like it's celebrating its third birthday this month. Makes me wonder if such issues as those described in the two Stack Overflow posts may have been fixed in that time.

More poking around showed me that, if that is indeed the issue (or even just part of the issue), it's probably not a simple drop-in operation to update okhttp in github-plugin and github-api-plugin. okhttp is now okhttp3, which brings along a requirement for Java 8. Possibly some other dependency updates too.

KostyaSha · 2019-02-14T01:49:29Z

The initial feature of cache i added in github-plugin, but with this ancient libraries better disable it.
okhttp's cache is caching 404 for X time, okhttp team doesn't support ancient (as usual in jenkins) version. Either you will fight with random issues. The other part of story is that github has delays between almost all operations (my test cases for example are full of polls https://github.com/KostyaSha/github-integration-plugin/tree/master/github-pullrequest-plugin/src/test/java/org/jenkinsci/plugins/github_integration/awaitility ).

The correct solution will be to start using fresh library and excluding cache for some error codes.

bitwiseman · 2019-02-14T01:52:10Z

@kshultzCB
Jenkins requires Java 8+ already, so no problem there. This does look like a notable change. 😮 But it also claims to have better handling of multipart.

kshultzCB · 2019-02-14T02:06:21Z

Yep, I helped out with Java-8-i-zation of gerrit-trigger, so I thought, maybe I could at least take a quick look. Didn't get much further along than that.

Thinking out loud: would it make sense to have the cache be opt-in? That's already been done for Windows because of a file naming problem. If it were opt-in, users could make a decision based on their needs. If they're hitting a lot of rate limit problems, they could turn it on, knowing there might be some tradeoffs. Maybe that might be a decent stopgap measure until some modernization can be done (Java8, okhttp3, etc).

rsandell · 2019-02-14T15:40:43Z

I've already bumped this plugin to Java 8 (hence the use of stream() in this pr) and KK's github-api seems ready for okhttp3?

kshultzCB · 2019-02-14T23:06:09Z

The one I was looking at that's not yet been bumped to Java 8 was github-plugin.

rsandell · 2019-02-19T11:20:21Z

Since the problem on the GitHub side seems to have magically disappeared during the last two weeks. Maybe we should merge this with the feature flag off by default?
We need to do something as the current master has the behaviour of re-throwing the fnf.

kshultzCB

Yeah, this is a good point at which to merge. Thanks, @rsandell 💯

[JENKINS-54126] if strange 404 when cache is on temporarily turn it o…

642fb38

…ff and try again

rsandell requested review from abayer, bitwiseman, dwnusbaum, kshultzCB and stephenc February 12, 2019 15:52

stephenc reviewed Feb 12, 2019

View reviewed changes

stephenc approved these changes Feb 12, 2019

View reviewed changes

dwnusbaum reviewed Feb 12, 2019

View reviewed changes

Comment thread src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMProbe.java

stephenc reviewed Feb 13, 2019

View reviewed changes

bitwiseman reviewed Feb 14, 2019

View reviewed changes

rsandell added 2 commits February 14, 2019 15:06

That pesky spotbugs warning

ed1f5cd

Toggles for the workaround

32710ee

rsandell closed this Feb 14, 2019

rsandell reopened this Feb 14, 2019

And a little logging

fbfde29

kshultzCB approved these changes Feb 19, 2019

View reviewed changes

bitwiseman approved these changes Feb 19, 2019

View reviewed changes

rsandell added 2 commits February 20, 2019 12:19

Turn the workaround off by default

5915dac

Missed fliping the flag back on for the tests

51bd388

rsandell merged commit 28603b8 into master Feb 20, 2019

rsandell deleted the JENKINS-54126 branch February 20, 2019 14:50

rsandell restored the JENKINS-54126 branch February 20, 2019 14:53

rsandell deleted the JENKINS-54126 branch February 20, 2019 14:53

kshultzCB mentioned this pull request Jan 15, 2020

[JENKINS-60353] Enable PR-204 workaround all the time #264

Merged

	final Optional<List<String>> status = Optional.ofNullable(fnf.getResponseHeaderFields() != null ? fnf.getResponseHeaderFields().get(null) : null);
	final Optional<List<String>> status = Optional.ofNullable(fnf.getResponseHeaderFields() != null ? Optional.ofNullable(fnf.getResponseHeaderFields().get(null)) : Optional.empty());

Conversation

rsandell commented Feb 12, 2019

Uh oh!

stephenc Feb 12, 2019

Choose a reason for hiding this comment

Uh oh!

stephenc left a comment

Choose a reason for hiding this comment

Uh oh!

stephenc Feb 12, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rsandell commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephenc Feb 13, 2019

Choose a reason for hiding this comment

Uh oh!

kshultzCB commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The relationship between strange 404s and missing branches/PRs

How are things going with this PR in place

Uh oh!

bitwiseman Feb 14, 2019

Choose a reason for hiding this comment

Uh oh!

rsandell Feb 14, 2019

Choose a reason for hiding this comment

Uh oh!

bitwiseman commented Feb 14, 2019

Uh oh!

kshultzCB commented Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KostyaSha commented Feb 14, 2019

Uh oh!

bitwiseman commented Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kshultzCB commented Feb 14, 2019

Uh oh!

rsandell commented Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kshultzCB commented Feb 14, 2019

Uh oh!

rsandell commented Feb 19, 2019

Uh oh!

kshultzCB left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rsandell commented Feb 13, 2019 •

edited

Loading

kshultzCB commented Feb 13, 2019 •

edited

Loading

kshultzCB commented Feb 14, 2019 •

edited

Loading

bitwiseman commented Feb 14, 2019 •

edited

Loading

rsandell commented Feb 14, 2019 •

edited

Loading