Skip to content

add Gradle Build Cache support with handler and tests#87

Merged
andrew merged 12 commits intogit-pkgs:mainfrom
matikepa:gradlew-cache-support
May 4, 2026
Merged

add Gradle Build Cache support with handler and tests#87
andrew merged 12 commits intogit-pkgs:mainfrom
matikepa:gradlew-cache-support

Conversation

@matikepa
Copy link
Copy Markdown
Contributor

@matikepa matikepa commented Apr 15, 2026

  • Accepts cache keys at /gradle/{key}
  • PUT stores cache entries in storage backend under _gradle/http-build-cache/{key}
  • GET returns stored artifact with content type application/vnd.gradle.build-cache-artifact.v2
  • HEAD returns headers/metadata without a response body
  • PUT returns 201 Created (including first write and overwrite)
  • Path traversal and malformed keys are rejected

Copy link
Copy Markdown
Contributor

@andrew andrew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a mention to the readme as well?

@matikepa matikepa requested a review from andrew April 20, 2026 08:28
Copy link
Copy Markdown
Contributor

@andrew andrew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the README section.

Bypassing the package database here is the right call. Build cache entries are opaque task-output blobs keyed by input hash, not packages, so there's nothing meaningful to record. But that same property means they churn fast: a busy CI writes hundreds of entries per build and old keys go dead the moment a source file changes. Gradle's own cache node and Develocity both run size-capped or TTL eviction for this reason. As written, _gradle/http-build-cache/ will grow without bound and nothing in the proxy will ever clean it up. I'd like to see an eviction story before this merges, even a simple one: a configurable max-age sweep, or a total-size cap that deletes oldest-first.

Related: handlePut (gradle.go:118) is the first endpoint in this proxy that accepts arbitrary client uploads, and it has no size limit and no auth. Anyone who can reach the proxy can PUT /gradle/anything and fill the storage backend. A configurable http.MaxBytesReader cap on the request body feels mandatory; a config flag to disable PUT (read-only cache mode) would be nice too. Auth can be a follow-up.

Smaller stuff:

  • gradle.go:31 NewGradleBuildCacheHandler(proxy *Proxy, _ string) discards its second arg. Drop the param and update the two call sites; unparam will flag this otherwise.
  • gradle.go:120 the Exists then Store dance is a TOCTOU and only exists to choose 200 vs 201. Gradle treats any 2xx as success, so drop Exists and always return 201. One fewer storage round-trip per write.
  • gradle.go:93 HEAD calls Storage.Open() then never reads. On S3 that's a wasted GetObject. Use Exists + Size for HEAD and only Open for GET.
  • gradle.go:140 X-Cache-Size is a custom header Gradle ignores; drop it unless something depends on it.
  • gradle.go:128 defer r.Body.Close() is redundant, net/http closes request bodies.
  • README points clients at /gradle/ with Kotlin DSL, dashboard.go:296 points at /gradle/cache/ with Groovy DSL. Pick one URL and one DSL for both.

Key validation, path-traversal handling, the _gradle/ storage prefix, and test coverage all look good.

…URL parameter and update related tests

Co-authored-by: Copilot <copilot@github.com>
@matikepa matikepa marked this pull request as draft April 27, 2026 20:34
- Introduced configuration options for Gradle build cache in config files and documentation.
- Implemented read-only mode and upload size limits for the Gradle build cache.
- Added cache eviction logic based on age and size, with corresponding tests.
- Enhanced storage interfaces to support listing objects by prefix.
@matikepa matikepa force-pushed the gradlew-cache-support branch from 8839465 to 17e1882 Compare April 27, 2026 20:55
@matikepa matikepa force-pushed the gradlew-cache-support branch from 17e1882 to 95a7927 Compare April 27, 2026 20:57
@matikepa matikepa marked this pull request as ready for review April 28, 2026 20:26
@matikepa matikepa requested a review from andrew April 28, 2026 20:27
@matikepa
Copy link
Copy Markdown
Contributor Author

matikepa commented Apr 29, 2026

There is also a potentially missing fallback case: Gradle plugin marker resolution may still be hitting Maven Central instead of the Gradle Plugin Portal, which can lead to 404 errors.

Should I include this fix in the current PR, or open a follow-up PR from a new branch?


bit more explanations below:

Technically, Gradle plugin resolution for plugins { id("...") version "..." } does not resolve the implementation artifact directly; it first requests a marker module at coordinate groupId:pluginId.gradle.plugin:version (typically a POM-only artifact).
In our previous flow, /maven artifact fetches were effectively sourced from Maven Central only, so marker requests that exist only in https://plugins.gradle.org/m2 could return 404 even when the corresponding implementation artifact (for example com.diffplug.spotless:spotless-plugin-gradle) was available.

The fix introduces targeted fallback behavior in Maven handling: for Gradle marker-shaped coordinates, if the primary Maven upstream fails, we retry the same path against the Gradle Plugin Portal and cache the result under the same Maven package/version key.
This preserves existing Maven behavior for normal artifacts, unblocks Gradle plugin marker resolution via proxy, and is validated by a live request and regression test using com.diffplug.spotless:com.diffplug.spotless.gradle.plugin:8.4.0 returning 200 and then serving from cache on subsequent calls.

Copy link
Copy Markdown
Contributor

@andrew andrew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All previous review items addressed. CI green. Code looks good.

Minor things that could be tidied up now or later:

  • Blob.ListPrefix fetches individual Attributes when ModTime.IsZero(). The eviction code already handles zero-ModTime entries gracefully, so this fallback could be dropped to avoid N+1 API calls on unusual providers.
  • handleGetOrHead calls both Open and Size for GET -- could skip Size and let chunked transfer handle it.
  • Filesystem.ListPrefix could use filepath.WalkDir instead of filepath.Walk to avoid extra stat calls.
  • PR description says "overwrite returns 200 OK" but code always returns 201. Code is correct, description is stale.

None blocking.

Mateusz (Mati) Kepa and others added 3 commits May 3, 2026 21:56
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
@andrew
Copy link
Copy Markdown
Contributor

andrew commented May 4, 2026

Thanks!

@andrew andrew merged commit 31a9ca7 into git-pkgs:main May 4, 2026
5 checks passed
@matikepa matikepa deleted the gradlew-cache-support branch May 4, 2026 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants