Skip to content

Commit 72aedbb

Browse files
bitwisemanbitwiseman
committed
JENKINS-54126 - Repro of github caching error
1 parent e7e3be6 commit 72aedbb

30 files changed

Lines changed: 1989 additions & 0 deletions

File tree

‎src/test/java/org/kohsuke/github/extras/okhttp3/GitHubCachingTest.java‎

Lines changed: 334 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,334 @@
1+
package org.kohsuke.github.extras.okhttp3;
2+
3+
import com.github.tomakehurst.wiremock.core.WireMockConfiguration;
4+
import com.github.tomakehurst.wiremock.extension.responsetemplating.ResponseTemplateTransformer;
5+
import okhttp3.Cache;
6+
import okhttp3.OkHttpClient;
7+
import org.apache.commons.io.FileUtils;
8+
import org.junit.Before;
9+
import org.junit.Ignore;
10+
import org.junit.Test;
11+
import org.kohsuke.github.AbstractGitHubWireMockTest;
12+
import org.kohsuke.github.GHContent;
13+
import org.kohsuke.github.GHException;
14+
import org.kohsuke.github.GHFileNotFoundException;
15+
import org.kohsuke.github.GHIssueState;
16+
import org.kohsuke.github.GHPullRequest;
17+
import org.kohsuke.github.GHRef;
18+
import org.kohsuke.github.GHRepository;
19+
import org.kohsuke.github.GitHub;
20+
21+
import java.io.File;
22+
import java.io.IOException;
23+
import java.util.List;
24+
25+
import static org.hamcrest.core.Is.is;
26+
27+
/**
28+
* Test showing the behavior of OkHttpConnector cache with GitHub 404 responses.
29+
*
30+
* @author Liam Newman
31+
*/
32+
public class GitHubCachingTest extends AbstractGitHubWireMockTest {
33+
34+
public GitHubCachingTest() {
35+
useDefaultGitHub = false;
36+
}
37+
38+
String testRefName = "heads/test/content_ref_cache";
39+
40+
@Override
41+
protected WireMockConfiguration getWireMockOptions() {
42+
return super.getWireMockOptions()
43+
.extensions(ResponseTemplateTransformer.builder().global(true).maxCacheEntries(0L).build());
44+
}
45+
46+
@Before
47+
public void setupRepo() throws Exception {
48+
if (mockGitHub.isUseProxy()) {
49+
for (GHPullRequest pr : getRepository(this.gitHubBeforeAfter).getPullRequests(GHIssueState.OPEN)) {
50+
pr.close();
51+
}
52+
try {
53+
GHRef ref = getRepository(this.gitHubBeforeAfter).getRef(testRefName);
54+
ref.delete();
55+
} catch (IOException e) {
56+
}
57+
}
58+
}
59+
60+
@Test
61+
public void OkHttpConnector_Cache_MaxAgeDefault_Zero_GitHubRef_Error_runnable() throws Exception {
62+
63+
requireProxy("This test method can be run locally for debugging and analyzing.");
64+
OkHttpConnector_Cache_MaxAgeDefault_Zero_GitHubRef_Error();
65+
}
66+
67+
@Ignore("The wiremock snapshot files attached to this test method show what was sent to and from the server during a run, but they aren't re-runnable - not templated.")
68+
@Test
69+
public void OkHttpConnector_Cache_MaxAgeDefault_Zero_GitHubRef_Error() throws Exception {
70+
71+
// requireProxy("For clarity. Will switch to snapshot shortly.");
72+
// snapshotNotAllowed();
73+
74+
OkHttpClient client = createClient(true);
75+
OkHttpConnector connector = new OkHttpConnector(client);
76+
77+
this.gitHub = getGitHubBuilder().withEndpoint(mockGitHub.apiServer().baseUrl())
78+
.withConnector(connector)
79+
.build();
80+
81+
// Alternate client also doing caching but staying in a good state
82+
// We use this to do sanity checks and other information gathering
83+
GitHub gitHub2 = getGitHubBuilder().withEndpoint(mockGitHub.apiServer().baseUrl())
84+
.withConnector(new OkHttpConnector(createClient(true)))
85+
.build();
86+
87+
// Create a branch from a known conflicting branch
88+
GHRepository repo = getRepository(gitHub);
89+
90+
String baseSha = repo.getRef("heads/test/unmergeable").getObject().getSha();
91+
92+
GHRef ref;
93+
ref = repo.createRef("refs/" + testRefName, baseSha);
94+
95+
// Verify we can query the created ref
96+
ref = repo.getRef(testRefName);
97+
98+
// Verify we can query the created ref from cache
99+
ref = repo.getRef(testRefName);
100+
101+
// Delete the ref
102+
ref.delete();
103+
104+
// This is just to show this isn't a race condition
105+
Thread.sleep(2000);
106+
107+
// Try to get the non-existant ref (GHFileNotFound)
108+
try {
109+
repo.getRef(testRefName);
110+
fail();
111+
} catch (GHFileNotFoundException e) {
112+
// expected
113+
114+
// FYI: Querying again when the item is actually not present does not produce a 304
115+
// It produces another 404,
116+
// Try to get the non-existant ref (GHFileNotFound)
117+
try {
118+
repo.getRef(testRefName);
119+
fail();
120+
} catch (GHFileNotFoundException ex) {
121+
// expected
122+
}
123+
124+
}
125+
126+
// This is just to show this isn't a race condition
127+
Thread.sleep(2000);
128+
129+
ref = repo.createRef("refs/" + testRefName, baseSha);
130+
131+
// Verify ref exists and can be queried from uncached connection
132+
// Expected: success
133+
// Actual: still GHFileNotFound due to caching: GitHub incorrectly returns 304
134+
// even though contents of the ref have changed.
135+
//
136+
// There source of this issue seems to be that 404's do not return an ETAG,
137+
// so the cache falls back to using "If-Modified-Since" which is erroneously returns a 304.
138+
//
139+
// NOTE: This is even worse than you might think: 404 responses don't return an ETAG, but 304 responses do.
140+
//
141+
// Due erroneous 304 returned from "If-Modified-Since", the ETAG returned by the first 304
142+
// is actually the ETAG for the NEW state of the ref query (the one where the ref exists).
143+
// This can be verified by comparing the ETAG from gitHub2 client to the ETAG in error.
144+
//
145+
// This means that server thinks it telling the client that the new state is stable
146+
// while the cache thinks it confirming the old state hasn't changed.
147+
//
148+
// So, after the first 304, the failure is locked in via ETAG and won't until the ref is modified again
149+
// or until the cache ages out entry without the URL being requeried (which is why users report that refreshing
150+
// is now help).
151+
152+
// Work arounds:
153+
154+
try {
155+
repo.getRef(testRefName);
156+
} catch (GHFileNotFoundException e) {
157+
// Sanity check: ref exists and can be queried from other client
158+
getRepository(gitHub2).getRef(testRefName);
159+
160+
// We're going to fail, query again to see the incorrect ETAG cached from first query being used
161+
// It is the same ETAG as the one returned to the other client.
162+
// Now we're in trouble.
163+
repo.getRef(testRefName);
164+
165+
}
166+
}
167+
168+
169+
@Ignore("Keeping for reference. Simpler repro above.")
170+
@Test
171+
public void OkHttpConnector_Cache_MaxAgeDefault_Zero_GitHubContents_Error() throws Exception {
172+
173+
// requireProxy("For clarity. Will switch to snapshot shortly.");
174+
// snapshotNotAllowed();
175+
176+
OkHttpClient client = createClient(true);
177+
OkHttpConnector connector = new OkHttpConnector(client);
178+
179+
this.gitHub = getGitHubBuilder().withEndpoint(mockGitHub.apiServer().baseUrl())
180+
.withConnector(connector)
181+
.build();
182+
183+
// Create a branch from a known conflicting branch
184+
GHRepository repo = getRepository(gitHub);
185+
186+
// Try to get a non-existant ref (GHFileNotFound)
187+
try {
188+
repo.getRef("heads/test/content_ref_cache");
189+
fail();
190+
} catch (GHFileNotFoundException e) {
191+
// ignore
192+
} catch (GHException e) {
193+
// ignore
194+
}
195+
196+
// Try to get the root directory contents for non-existant ref (GHFileNotFound)
197+
try {
198+
repo.getDirectoryContent("/", "refs/heads/test/content_ref_cache");
199+
fail();
200+
} catch (GHFileNotFoundException e) {
201+
// ignore
202+
} catch (GHException e) {
203+
// ignore
204+
}
205+
206+
GHRef ref = repo.createRef("refs/heads/test/content_ref_cache",
207+
repo.getRef("heads/test/unmergeable").getObject().getSha());
208+
209+
// Wait a little to make sure there's some time between queries
210+
Thread.sleep(5000);
211+
212+
// Verify we can query the created ref
213+
repo.getRef("heads/test/content_ref_cache");
214+
215+
// Sanity check: ref exists and can be queried from uncached connection
216+
// if (mockGitHub.isUseProxy()) {
217+
// getRepository(this.gitHubBeforeAfter).getDirectoryContent("/", ref.getRef());
218+
// }
219+
220+
// Verify ref exists and can be queried from uncached connection
221+
// Expected: success
222+
// Actual: still GHFileNotFound due to caching: GitHub incorrectly returns 304
223+
// even though contents of the ref have changed.
224+
try {
225+
repo.getDirectoryContent("/", ref.getRef());
226+
} catch (GHFileNotFoundException e) {
227+
// Useful for breakpoint when debugging
228+
throw e;
229+
} catch (GHException e) {
230+
// Useful for breakpoint when debugging
231+
throw e;
232+
}
233+
234+
}
235+
236+
@Ignore("Keeping for reference. Simpler repro above.")
237+
@Test
238+
public void OkHttpConnector_Cache_MaxAgeDefault_Zero_PRGitHubContents_Error() throws Exception {
239+
240+
OkHttpClient client = createClient(true);
241+
OkHttpConnector connector = new OkHttpConnector(client);
242+
243+
this.gitHub = getGitHubBuilder().withEndpoint(mockGitHub.apiServer().baseUrl())
244+
.withConnector(connector)
245+
.build();
246+
247+
if (mockGitHub.isUseProxy()) {
248+
for (GHPullRequest pr : getRepository(this.gitHubBeforeAfter).getPullRequests(GHIssueState.OPEN)) {
249+
pr.close();
250+
}
251+
try {
252+
GHRef ref = getRepository(this.gitHubBeforeAfter).getRef("heads/test/content_ref_cache");
253+
ref.delete();
254+
} catch (IOException e) {
255+
}
256+
}
257+
258+
// Create a branch from a known conflicting branch
259+
GHRepository repo = getRepository(gitHub);
260+
261+
GHRef ref = repo.createRef("refs/heads/test/content_ref_cache",
262+
repo.getRef("heads/test/unmergeable").getObject().getSha());
263+
264+
// Ensure ref exists and can be queried
265+
repo.getDirectoryContent("/", ref.getRef());
266+
267+
// Create a PR from the created (unmergeable) branch
268+
GHPullRequest pr = repo.createPullRequest("Title", ref.getRef(), "master", "");
269+
270+
// Verify branch is unmergable state true
271+
while (pr.getMergeable() == null) {
272+
Thread.sleep(500);
273+
}
274+
assertThat(pr.getMergeable(), is(false));
275+
276+
String mergeRefName = "pull/" + Integer.toString(pr.getNumber()) + "/merge";
277+
278+
// Try to get the root directory contents for non-existant merge ref (GHFileNotFound)
279+
try {
280+
repo.getDirectoryContent("/", "refs/" + mergeRefName);
281+
fail();
282+
} catch (GHFileNotFoundException e) {
283+
// ignore
284+
} catch (GHException e) {
285+
// ignore
286+
}
287+
288+
// Make PR mergeable
289+
ref.updateTo(repo.getRef("heads/test/mergeable_branch").getObject().getSha(), true);
290+
pr.refresh();
291+
// Verify mergable state true
292+
while (pr.getMergeable() == null) {
293+
Thread.sleep(500);
294+
}
295+
assertThat(pr.getMergeable(), is(true));
296+
297+
// Verify we can get the root directory contents for merge ref
298+
// Expected: success
299+
// Actual: still GHFileNotFound due to caching - GitHub server error
300+
try {
301+
List<GHContent> files = repo.getDirectoryContent("/", "refs/" + mergeRefName);
302+
} catch (GHFileNotFoundException e) {
303+
// Useful for breakpoint when debugging
304+
throw e;
305+
} catch (GHException e) {
306+
// Useful for breakpoint when debugging
307+
throw e;
308+
}
309+
310+
}
311+
312+
private static int clientCount = 0;
313+
314+
private OkHttpClient createClient(boolean useCache) throws IOException {
315+
OkHttpClient.Builder builder = new OkHttpClient().newBuilder();
316+
317+
if (useCache) {
318+
File cacheDir = new File("target/cache/" + baseFilesClassPath + "/" + mockGitHub.getMethodName()
319+
+ Integer.toString(clientCount++));
320+
cacheDir.mkdirs();
321+
FileUtils.cleanDirectory(cacheDir);
322+
Cache cache = new Cache(cacheDir, 100 * 1024L * 1024L);
323+
324+
builder.cache(cache);
325+
}
326+
327+
return builder.build();
328+
}
329+
330+
private static GHRepository getRepository(GitHub gitHub) throws IOException {
331+
return gitHub.getOrganization("github-api-test-org").getRepository("github-api");
332+
}
333+
334+
}

‎src/test/java/org/kohsuke/github/junit/GitHubWireMockRule.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,7 @@ protected void after() {
114114
this.apiServer()
115115
.snapshotRecord(recordSpec().forTarget("https://api.github.com")
116116
.captureHeader("If-None-Match")
117+
.captureHeader("If-Modified-Since")
117118
.captureHeader("Accept")
118119
.extractTextBodiesOver(255));
119120

@@ -124,6 +125,7 @@ protected void after() {
124125
this.rawServer()
125126
.snapshotRecord(recordSpec().forTarget("https://raw.githubusercontent.com")
126127
.captureHeader("If-None-Match")
128+
.captureHeader("If-Modified-Since")
127129
.captureHeader("Accept")
128130
.extractTextBodiesOver(255));
129131

@@ -135,6 +137,7 @@ protected void after() {
135137
this.uploadsServer()
136138
.snapshotRecord(recordSpec().forTarget("https://uploads.github.com")
137139
.captureHeader("If-None-Match")
140+
.captureHeader("If-Modified-Since")
138141
.captureHeader("Accept")
139142
.extractTextBodiesOver(255));
140143

0 commit comments

Comments
 (0)