Preserve the HTTP response cache when evicting unused connections - #1548
Open
joshfree wants to merge 3 commits into
Open
Preserve the HTTP response cache when evicting unused connections#1548joshfree wants to merge 3 commits into
joshfree wants to merge 3 commits into
Conversation
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#1547 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4959e1f9-f8e6-4e97-a487-f395a0123c79
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#1547 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4959e1f9-f8e6-4e97-a487-f395a0123c79
srnagar
reviewed
Aug 6, 2026
anannya03
reviewed
Aug 6, 2026
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
Author
|
Gentle nudge on this one. CI has been fully green since 2026-08-07 (all 15 ci.jenkins.io checks, Java 8/11 on Linux and Windows), the branch is rebased on current master, and the feedback from the initial review pass has been addressed with the threads resolved. The change fixes ETag cache reuse across scans (ref #1547) and includes added test coverage. Ready for maintainer review whenever there is bandwidth; any further feedback will be addressed promptly. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Connectorpools aGitHubconnection per API URL and credential, each backed by an OkHttp on-disk response cache.UnusedConnectionDestroyerevicts a connection once it has been idle for 30 minutes, which normally happens between folder scans.On eviction,
GitHubConnection.removeAllUnusedhandled the cache only for GitHub App connections, callingCache.delete()to discard the cache directory. Connections for other credential kinds neither deleted nor closed the cache. In both cases the stored ETags were unavailable to the next connection, so the following scan refetched every resource with a full200response instead of revalidating with a conditional request.This PR closes the cache on eviction for every credential type without deleting it: the cache is closed to release its file handles and flush its journal, and the directory is preserved so the next connection can reuse the stored ETags and revalidate with conditional requests (
304 Not Modified).See #1547 for further information.
Testing done
Added
ConnectorTest, which populates a real OkHttp cache with a cacheable response, evicts it viaConnector.evictConnectionCache, and asserts that the cache directory is preserved, the cache is closed, and the stored entry survives for reuse. It also asserts that evicting a connection without a cache (caching disabled) is a no-op.mvn test -Dtest=ConnectorTestpasses;GitHubSCMProbeTest(which exercises the cache path) also passes.Submitter checklist
Reviewer checklist
Documentation changes
No documentation changes are needed: this fixes internal cache lifecycle behavior with no change to configuration or public API.
Users/aliases to notify