Skip to content

ETag cache not reused across folder scans — conditional requests never sent for pull-request listing #1547

Description

@joshfree

Summary

On each Multibranch/Organization Folder scan, the plugin lists open PRs via GET /repos/{owner}/{repo}/pulls?state=open. The github-api client is configured with an OkHttp cache that supports conditional requests (If-None-Match/ETag304), but the cache is not reused between scans — so nearly every scan sends an unconditional request and gets a full 200 even when the list is unchanged.

Cause: a pooled connection (and its OkHttp Cache) is discarded after 30 min idle, while scans run less often (hourly by default). The next scan can't reuse the previous scan's cache.

Details (master @ fa27ed96)

UnusedConnectionDestroyer evicts pooled connections idle > 30 min (L619-634). On eviction, removeAllUnused() handles the Cache by credential type (L694-715):

  1. GitHub App creds: cleanupCacheFolder=true (L423) → eviction runs cache.delete() (L700-702), so the next scan starts from an empty directory.
  2. PAT / other creds: cleanupCacheFolder=false, so that branch is skipped and the Cache is neither deleted nor close()d — it's dropped still-open. The next scan opens a new Cache on the same directory; it's unclear whether entries from the prior (never-closed) Cache are reliably reused here, and conditional requests do not appear to be sent in this path. (Worth confirming with a reproduction.)

The cache is also disabled by default on Windows (cacheSize = isWindows() ? 0 : 20, L168-170).

Why it matters

GitHub doesn't count 304 responses against the REST rate limit (docs). Reusing the cache means unchanged PR lists return 304 — no quota spent, no body transferred. For controllers scanning many repos on a short interval, that lowers rate-limit consumption (and the back-off it triggers) and cuts redundant transfer/parsing.

Benefit scales with how often listings are unchanged; changed listings still return 200 as today. This restores existing-but-inert caching and doesn't change scan results.

Proposed fix

1. Preserve the cache between scans. On eviction, always close() the Cache (flush + release) but never delete(), for all credential types. It's already cacheSize-bounded (LRU) and keyed by a stable per-credential hash, so keeping it on disk is bounded/safe:

- if (record.cache != null && record.cleanupCacheFolder) {
-     record.cache.delete();
-     record.cache.close();
- }
+ if (record.cache != null) {
+     record.cache.close();
+ }

The next scan can then reopen a warm cache and send If-None-Match. The cleanupCacheFolder flag can be removed.

2. (Optional) Enable the cache on Windows — default cacheSize to 20 instead of isWindows() ? 0 : 20.

Validation

Regression test: prime the cache, force eviction, re-request, assert the cache is reused / If-None-Match is sent — for both GitHub App and PAT credentials.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions