diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/Connector.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/Connector.java index 8a8a52eae..e974e399c 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/Connector.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/Connector.java @@ -56,10 +56,12 @@ import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; import java.util.Base64; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Random; +import java.util.Set; import java.util.WeakHashMap; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; @@ -94,6 +96,15 @@ public class Connector { private static final Map> checked = new WeakHashMap<>(); private static final long API_URL_REVALIDATE_MILLIS = TimeUnit.MINUTES.toMillis(5); + /** + * How long a cache directory may go untouched before {@link #pruneStaleCaches()} removes it, once + * it no longer backs a pooled connection. Overridable via the {@code + * org.jenkinsci.plugins.github_branch_source.GitHubSCMSource.cacheStaleThresholdMillis} system + * property. + */ + private static final long CACHE_STALE_THRESHOLD_MILLIS = SystemProperties.getLong( + GitHubSCMSource.class.getName() + ".cacheStaleThresholdMillis", TimeUnit.DAYS.toMillis(7)); + private static final Random ENTROPY = new Random(); private static final String SALT = Long.toHexString(ENTROPY.nextLong()); private static final OkHttpClient baseClient = @@ -406,7 +417,8 @@ public static ListBoxModel listCheckoutCredentials(@CheckForNull Item context, S GitHubConnection record = GitHubConnection.lookup(connectionId, () -> { try { - Cache cache = getCache(jenkins, apiUrl, authHash, username); + File cacheDir = getCacheDir(jenkins, apiUrl, authHash, username); + Cache cache = createCache(cacheDir); GitHubBuilder gb = createGitHubBuilder(apiUrl, cache); @@ -420,7 +432,7 @@ public static ListBoxModel listCheckoutCredentials(@CheckForNull Item context, S gb.withAuthorizationProvider( ImmutableAuthorizationProvider.fromLoginAndPassword(username, password)); } - return new GitHubConnection(gb.build(), cache, credentials instanceof GitHubAppCredentials); + return new GitHubConnection(gb.build(), cache, cacheDir); } catch (IOException e) { throw new RuntimeException(e.getMessage(), e); } @@ -463,36 +475,99 @@ private static GitHubBuilder createGitHubBuilder(@NonNull String apiUrl, @CheckF return gb; } + @NonNull + private static File getCacheBaseDir(@NonNull Jenkins jenkins) { + String cacheRootDir = SystemProperties.getString(GitHubSCMSource.class.getName() + ".cacheRootDir"); + return cacheRootDir != null + ? new File(cacheRootDir) + : new File(jenkins.getRootDir(), GitHubSCMProbe.class.getName() + ".cache"); + } + + /** + * Computes the stable on-disk cache directory for a connection, or {@code null} when caching is + * disabled or unavailable. The directory name is a hash of the endpoint, username and credential + * material (for GitHub App credentials the App id, accessible repositories, permissions and + * private key — not the short-lived installation token), so the same credential maps to the same + * directory on every scan and its cache is reused across scans rather than rebuilt. + */ @CheckForNull - private static Cache getCache( + private static File getCacheDir( @NonNull Jenkins jenkins, @NonNull String apiUrl, @NonNull String authHash, @CheckForNull String username) { - Cache cache = null; - int cacheSize = GitHubSCMSource.getCacheSize(); - if (cacheSize > 0) { - String cacheRootDir = SystemProperties.getString(GitHubSCMSource.class.getName() + ".cacheRootDir"); - File cacheBase = cacheRootDir != null - ? new File(cacheRootDir) - : new File(jenkins.getRootDir(), GitHubSCMProbe.class.getName() + ".cache"); - File cacheDir = null; - try { - MessageDigest sha256 = MessageDigest.getInstance("SHA-256"); - sha256.update(apiUrl.getBytes(StandardCharsets.UTF_8)); - sha256.update("::".getBytes(StandardCharsets.UTF_8)); - if (username != null) { - sha256.update(username.getBytes(StandardCharsets.UTF_8)); - } - sha256.update("::".getBytes(StandardCharsets.UTF_8)); - sha256.update(authHash.getBytes(StandardCharsets.UTF_8)); - cacheDir = new File( - cacheBase, Base64.getUrlEncoder().withoutPadding().encodeToString(sha256.digest())); - } catch (NoSuchAlgorithmException e) { - // no cache for you mr non-spec compliant JVM + if (GitHubSCMSource.getCacheSize() <= 0) { + return null; + } + File cacheBase = getCacheBaseDir(jenkins); + try { + MessageDigest sha256 = MessageDigest.getInstance("SHA-256"); + sha256.update(apiUrl.getBytes(StandardCharsets.UTF_8)); + sha256.update("::".getBytes(StandardCharsets.UTF_8)); + if (username != null) { + sha256.update(username.getBytes(StandardCharsets.UTF_8)); + } + sha256.update("::".getBytes(StandardCharsets.UTF_8)); + sha256.update(authHash.getBytes(StandardCharsets.UTF_8)); + return new File(cacheBase, Base64.getUrlEncoder().withoutPadding().encodeToString(sha256.digest())); + } catch (NoSuchAlgorithmException e) { + // no cache for you mr non-spec compliant JVM + return null; + } + } + + @CheckForNull + private static Cache createCache(@CheckForNull File cacheDir) { + if (cacheDir == null) { + return null; + } + return new Cache(cacheDir, GitHubSCMSource.getCacheSize() * 1024L * 1024L); + } + + /** + * Prunes cache directories that no longer back a pooled connection and have gone untouched for at + * least {@link #CACHE_STALE_THRESHOLD_MILLIS}. + * + *

Because a cache directory is named by a stable hash of the endpoint, username and credential + * material, it is reused (and its modification time refreshed) on every scan for as long as those + * inputs are unchanged. When they change — for example a GitHub App's accessible repositories or + * permissions are edited — the previous directory is never selected again. Such orphaned + * directories would otherwise accumulate on disk, so they are removed here once stale. A directory + * that still backs a pooled connection is never removed regardless of age. + */ + static void pruneStaleCaches() { + Jenkins jenkins = Jenkins.getInstanceOrNull(); + if (jenkins == null) { + return; + } + Set liveDirs = new HashSet<>(); + for (GitHubConnection connection : connections.values()) { + if (connection.cacheDir != null) { + liveDirs.add(connection.cacheDir); } - if (cacheDir != null) { - cache = new Cache(cacheDir, cacheSize * 1024L * 1024L); + } + pruneStaleCaches(getCacheBaseDir(jenkins), liveDirs, System.currentTimeMillis() - CACHE_STALE_THRESHOLD_MILLIS); + } + + static void pruneStaleCaches(@CheckForNull File cacheBase, @NonNull Set liveDirs, long staleBefore) { + if (cacheBase == null || !cacheBase.isDirectory()) { + return; + } + File[] entries = cacheBase.listFiles(); + if (entries == null) { + return; + } + for (File dir : entries) { + if (!dir.isDirectory() || liveDirs.contains(dir)) { + continue; + } + if (dir.lastModified() >= staleBefore) { + // Recently used: keep so the next scan for the same credential can revalidate. + continue; + } + try { + Util.deleteRecursive(dir); + } catch (IOException e) { + LOGGER.log(WARNING, "Exception pruning stale cache directory: " + dir, e); } } - return cache; } public static void release(@CheckForNull GitHub hub) { @@ -515,6 +590,29 @@ public static void release(@CheckForNull GitHub hub) { } } + /** + * Closes the on-disk HTTP response cache of a connection that is being evicted from the pool. + * + *

The cache is closed so that its file handles are released and its journal is flushed, but + * the cache directory is intentionally not deleted. Preserving the directory lets the + * next connection created for the same credentials reuse the stored ETags and revalidate with + * conditional requests (returning {@code 304 Not Modified}) instead of refetching every + * resource from scratch on the following scan. + * + * @param cache the cache to close, or {@code null} if the connection had no cache + * @param connectionId identifies the connection, used only for logging + */ + static void evictConnectionCache(@CheckForNull Cache cache, @NonNull Object connectionId) { + if (cache == null) { + return; + } + try { + cache.close(); + } catch (IOException e) { + LOGGER.log(WARNING, "Exception closing cache for unused connection: " + connectionId, e); + } + } + static List githubDomainRequirements(String apiUri) { return URIRequirementBuilder.fromUri(StringUtils.defaultIfEmpty(apiUri, GitHubServerConfig.GITHUB_URL)) .build(); @@ -631,6 +729,7 @@ protected void doRun() throws Exception { long unusedThreshold = System.currentTimeMillis() - TimeUnit.MINUTES.toMillis(30); GitHubConnection.removeAllUnused(unusedThreshold); + pruneStaleCaches(); } } @@ -641,15 +740,17 @@ static class GitHubConnection { @CheckForNull private final Cache cache; - private final boolean cleanupCacheFolder; + @CheckForNull + private final File cacheDir; + private final AtomicInteger usageCount = new AtomicInteger(1); private final AtomicLong lastUsed = new AtomicLong(System.currentTimeMillis()); private long lastVerified = Long.MIN_VALUE; - private GitHubConnection(GitHub gitHub, Cache cache, boolean cleanupCacheFolder) { + private GitHubConnection(GitHub gitHub, Cache cache, File cacheDir) { this.gitHub = gitHub; this.cache = cache; - this.cleanupCacheFolder = cleanupCacheFolder; + this.cacheDir = cacheDir; } /** @@ -696,14 +797,7 @@ private static void removeAllUnused(long threshold) throws IOException { connections.computeIfPresent(connectionId, (id, record) -> { long lastUse = record.lastUsed.get(); if (record.usageCount.get() == 0 && lastUse < threshold) { - try { - if (record.cache != null && record.cleanupCacheFolder) { - record.cache.delete(); - record.cache.close(); - } - } catch (IOException e) { - LOGGER.log(WARNING, "Exception removing cache directory for unused connection: " + id, e); - } + evictConnectionCache(record.cache, id); reverseLookup.remove(record.gitHub); // returning null will remove the connection diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/ConnectorTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/ConnectorTest.java new file mode 100644 index 000000000..d71a82dae --- /dev/null +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/ConnectorTest.java @@ -0,0 +1,171 @@ +package org.jenkinsci.plugins.github_branch_source; + +import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; +import static com.github.tomakehurst.wiremock.client.WireMock.equalTo; +import static com.github.tomakehurst.wiremock.client.WireMock.get; +import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; +import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.greaterThan; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; +import static org.junit.Assert.assertThrows; + +import com.github.tomakehurst.wiremock.core.WireMockConfiguration; +import com.github.tomakehurst.wiremock.junit.WireMockRule; +import java.io.File; +import java.util.Collections; +import java.util.concurrent.TimeUnit; +import okhttp3.Cache; +import okhttp3.OkHttpClient; +import okhttp3.Request; +import okhttp3.Response; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +/** + * Tests for the on-disk HTTP response cache lifecycle when unused connections are evicted from the + * pool by {@link Connector.UnusedConnectionDestroyer}. + */ +public class ConnectorTest { + + @Rule + public TemporaryFolder tmp = new TemporaryFolder(); + + @Rule + public WireMockRule server = + new WireMockRule(WireMockConfiguration.options().dynamicPort()); + + @Test + public void evictingConnectionClosesCacheButPreservesDirectoryForReuse() throws Exception { + server.stubFor(get(urlEqualTo("/data")) + .willReturn(aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/json") + .withHeader("Cache-Control", "max-age=60") + .withHeader("ETag", "\"v1\"") + .withBody("{\"hello\":\"world\"}"))); + + File cacheDir = new File(tmp.getRoot(), "response-cache"); + Cache cache = new Cache(cacheDir, 10L * 1024 * 1024); + + // Populate the cache with a real, cacheable response so its on-disk journal exists. + OkHttpClient client = new OkHttpClient.Builder().cache(cache).build(); + Request request = new Request.Builder().url(server.baseUrl() + "/data").build(); + try (Response response = client.newCall(request).execute()) { + response.body().string(); + } + assertThat("the request should have populated the on-disk cache", cacheDir.isDirectory(), is(true)); + + Connector.evictConnectionCache(cache, "test-connection"); + + assertThat( + "the cache directory must be preserved after eviction so the next connection can " + + "reuse the stored ETags for conditional requests", + cacheDir.isDirectory(), + is(true)); + + // The cache must have been closed, so further use of the evicted instance fails. + assertThrows(IllegalStateException.class, cache::flush); + + // The stored entry survives on disk: a fresh cache over the same directory still holds it. + Cache reopened = new Cache(cacheDir, 10L * 1024 * 1024); + try { + assertThat("the cached response should survive eviction", reopened.size(), greaterThan(0L)); + } finally { + reopened.close(); + } + } + + @Test + public void evictingConnectionWithoutCacheIsANoOp() { + // A connection created while caching is disabled has no cache; eviction must not fail. + Connector.evictConnectionCache(null, "test-connection-without-cache"); + } + + @Test + public void reopeningCacheRevalidatesWithConditionalRequest() throws Exception { + // A revalidation carrying the stored ETag is answered with 304 Not Modified. + server.stubFor(get(urlEqualTo("/data")) + .atPriority(1) + .withHeader("If-None-Match", equalTo("\"v1\"")) + .willReturn(aResponse().withStatus(304))); + // The initial response is storable but must be revalidated before reuse, and carries an ETag. + server.stubFor(get(urlEqualTo("/data")) + .atPriority(2) + .willReturn(aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/json") + .withHeader("Cache-Control", "no-cache") + .withHeader("ETag", "\"v1\"") + .withBody("{\"hello\":\"world\"}"))); + + File cacheDir = new File(tmp.getRoot(), "response-cache"); + Request request = new Request.Builder().url(server.baseUrl() + "/data").build(); + + // Populate the cache, then close it as connection eviction would. + Cache cache = new Cache(cacheDir, 10L * 1024 * 1024); + OkHttpClient client = new OkHttpClient.Builder().cache(cache).build(); + try (Response response = client.newCall(request).execute()) { + assertThat(response.code(), is(200)); + assertThat(response.body().string(), is("{\"hello\":\"world\"}")); + } + Connector.evictConnectionCache(cache, "test-connection"); + + // Reopen a fresh cache over the same directory: the persisted ETag must drive a conditional + // request whose 304 response is served from the on-disk cache rather than refetched in full. + Cache reopened = new Cache(cacheDir, 10L * 1024 * 1024); + OkHttpClient reopenedClient = new OkHttpClient.Builder().cache(reopened).build(); + try (Response response = reopenedClient.newCall(request).execute()) { + assertThat("a 304 revalidation should be served from cache as a 200", response.code(), is(200)); + assertThat(response.body().string(), is("{\"hello\":\"world\"}")); + assertThat( + "the response should have been served from the on-disk cache", + response.cacheResponse(), + is(notNullValue())); + assertThat( + "the revalidation must have hit the network with a 304", + response.networkResponse(), + is(notNullValue())); + assertThat(response.networkResponse().code(), is(304)); + } finally { + reopened.close(); + } + + // The server must have received a revalidation carrying the stored ETag. + server.verify(getRequestedFor(urlEqualTo("/data")).withHeader("If-None-Match", equalTo("\"v1\""))); + } + + @Test + public void pruneStaleCachesRemovesOnlyStaleOrphanDirectories() throws Exception { + File cacheBase = tmp.newFolder("caches"); + File live = new File(cacheBase, "live"); + File staleOrphan = new File(cacheBase, "stale-orphan"); + File freshOrphan = new File(cacheBase, "fresh-orphan"); + for (File d : new File[] {live, staleOrphan, freshOrphan}) { + assertThat(d.mkdirs(), is(true)); + // A file inside gives the recursive delete something to remove. + assertThat(new File(d, "marker").createNewFile(), is(true)); + } + long now = System.currentTimeMillis(); + long tenDaysAgo = now - TimeUnit.DAYS.toMillis(10); + live.setLastModified(tenDaysAgo); + staleOrphan.setLastModified(tenDaysAgo); + freshOrphan.setLastModified(now); + + long staleBefore = now - TimeUnit.DAYS.toMillis(7); + Connector.pruneStaleCaches(cacheBase, Collections.singleton(live), staleBefore); + + assertThat("a directory backing a live connection must be kept", live.isDirectory(), is(true)); + assertThat("a recently used directory must be kept for reuse", freshOrphan.isDirectory(), is(true)); + assertThat("a stale orphaned directory must be pruned", staleOrphan.exists(), is(false)); + } + + @Test + public void pruneStaleCachesToleratesMissingBaseDirectory() { + File missing = new File(tmp.getRoot(), "does-not-exist"); + // Must not throw when the cache base directory has never been created. + Connector.pruneStaleCaches(missing, Collections.emptySet(), System.currentTimeMillis()); + } +}