From 168150ab2481d6bb14f32ab28d7583213024b9aa Mon Sep 17 00:00:00 2001 From: Josh Free Date: Wed, 5 Aug 2026 13:15:33 -0700 Subject: [PATCH 1/3] Preserve the HTTP response cache when evicting unused connections Connector pools a GitHub connection per API URL and credential, each with an OkHttp on-disk response cache. UnusedConnectionDestroyer evicts a connection after it has been idle for 30 minutes. Because a folder scan typically runs less often than that, a connection is usually evicted between scans. On eviction, GitHub App connections called Cache.delete(), discarding the cache directory, while connections for other credential kinds neither deleted nor closed the cache. Either way the stored ETags were not available to the next connection, so the following scan refetched every resource with a full 200 response instead of revalidating with a conditional request. Close the cache on eviction for every credential type, without deleting it. The cache is closed to release its file handles and flush its journal; keeping the directory lets the next connection reuse the stored ETags and revalidate with conditional requests (304 Not Modified). Refs jenkinsci/github-branch-source-plugin#1547 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4959e1f9-f8e6-4e97-a487-f395a0123c79 --- .../github_branch_source/Connector.java | 38 +++++++++++++------ 1 file changed, 26 insertions(+), 12 deletions(-) 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..37a9ca018 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 @@ -420,7 +420,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); } catch (IOException e) { throw new RuntimeException(e.getMessage(), e); } @@ -515,6 +515,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(); @@ -641,15 +664,13 @@ static class GitHubConnection { @CheckForNull private final Cache cache; - private final boolean cleanupCacheFolder; 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) { this.gitHub = gitHub; this.cache = cache; - this.cleanupCacheFolder = cleanupCacheFolder; } /** @@ -696,14 +717,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 From 224911073d1e221918fd2fa2bdd291ba86dca9ff Mon Sep 17 00:00:00 2001 From: Josh Free Date: Wed, 5 Aug 2026 13:15:50 -0700 Subject: [PATCH 2/3] Add ConnectorTest for cache reuse across connection eviction Cover the response-cache lifecycle exercised by Connector.evictConnectionCache: after a connection populates its cache and is evicted, the cache directory must be preserved (so stored ETags remain available for the next connection) and the cache must be closed. Also verify that evicting a connection without a cache is a no-op, which happens when caching is disabled. Refs jenkinsci/github-branch-source-plugin#1547 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4959e1f9-f8e6-4e97-a487-f395a0123c79 --- .../github_branch_source/ConnectorTest.java | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 src/test/java/org/jenkinsci/plugins/github_branch_source/ConnectorTest.java 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..8a4dd256b --- /dev/null +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/ConnectorTest.java @@ -0,0 +1,81 @@ +package org.jenkinsci.plugins.github_branch_source; + +import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; +import static com.github.tomakehurst.wiremock.client.WireMock.get; +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.junit.Assert.assertThrows; + +import com.github.tomakehurst.wiremock.core.WireMockConfiguration; +import com.github.tomakehurst.wiremock.junit.WireMockRule; +import java.io.File; +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"); + } +} From 1bfb12b66dbf7edfd3f5987179fdc4ebf01b785d Mon Sep 17 00:00:00 2001 From: Josh Free Date: Thu, 6 Aug 2026 21:09:40 -0700 Subject: [PATCH 3/3] Prune orphaned response caches and cover conditional revalidation A connection's response cache directory is named by a stable 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 directory is reused across scans 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 and, now that evicting a connection no longer deletes its cache, would remain on disk indefinitely. UnusedConnectionDestroyer now also prunes cache directories that no longer back a pooled connection once they have gone untouched for a configurable threshold (7 days by default), and never removes a directory that still backs a live connection or has been used recently. Adds tests for the pruning rules and for conditional revalidation (If-None-Match / 304 served from cache) after a cache is closed and reopened over the same directory. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4959e1f9-f8e6-4e97-a487-f395a0123c79 --- .../github_branch_source/Connector.java | 136 ++++++++++++++---- .../github_branch_source/ConnectorTest.java | 90 ++++++++++++ 2 files changed, 198 insertions(+), 28 deletions(-) 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 37a9ca018..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); + 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)); } - if (cacheDir != null) { - cache = new Cache(cacheDir, cacheSize * 1024L * 1024L); + 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); + } + } + 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) { @@ -654,6 +729,7 @@ protected void doRun() throws Exception { long unusedThreshold = System.currentTimeMillis() - TimeUnit.MINUTES.toMillis(30); GitHubConnection.removeAllUnused(unusedThreshold); + pruneStaleCaches(); } } @@ -664,13 +740,17 @@ static class GitHubConnection { @CheckForNull private final Cache cache; + @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) { + private GitHubConnection(GitHub gitHub, Cache cache, File cacheDir) { this.gitHub = gitHub; this.cache = cache; + this.cacheDir = cacheDir; } /** 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 index 8a4dd256b..d71a82dae 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/ConnectorTest.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/ConnectorTest.java @@ -1,16 +1,21 @@ 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; @@ -78,4 +83,89 @@ 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()); + } }