Merged
Conversation
Chart Preview Readyhelm install test oci://ghcr.io/eduide/charts/theia-shared-cache --version 0.4.0-pr.34Updated: e8c1644 |
📝 WalkthroughWalkthroughAdds Bazel remote cache support (AC and CAS) with GET/PUT handlers, SHA-256 validation, CAS hash verification toggle, OpenTelemetry metrics, config + defaults for cache behavior, and Helm chart bump to 0.4.0 including values for cache settings. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as BazelHandler
participant Storage
participant Metrics
rect rgba(100, 150, 200, 0.5)
Note over Client,Metrics: GET Flow (Retrieval)
Client->>Server: GET /bazel/{ac|cas}/:hash
Server->>Server: Validate SHA-256 hex format
Server->>Storage: Get(ctx, hash)
alt Cache Hit
Storage-->>Server: reader, size
Server->>Metrics: Record cache hit
Server->>Client: 200 OK + data
else Cache Miss
Storage-->>Server: ErrNotFound
Server->>Metrics: Record cache miss
Server->>Client: 404 Not Found
else Error
Storage-->>Server: error
Server->>Client: 500 Internal Error
end
end
rect rgba(200, 150, 100, 0.5)
Note over Client,Metrics: PUT Flow (Storage)
Client->>Server: PUT /bazel/{ac|cas}/:hash + body
Server->>Server: Validate SHA-256 hex format
Server->>Server: Enforce size limits
Server->>Server: Read request body
alt CAS with verification
Server->>Server: Compute SHA-256 of body
Server->>Server: Compare with provided hash
alt Mismatch
Server->>Metrics: Record hash mismatch
Server->>Client: 400 Bad Request
end
end
Server->>Storage: Put(ctx, hash, data)
Server->>Metrics: Record entry size
alt Success
Server->>Client: 200 OK
else Size exceeded
Server->>Client: 413 Payload Too Large
else Failure
Server->>Client: 500 Internal Error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/internal/server/server.go (1)
95-99: Type assertion fails fatally during startup.The
Fatalcall prevents graceful degradation if the storage backend doesn't support namespaces. This is acceptable for a required feature, but consider if this should be a startup validation check inmain()rather than during route setup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/internal/server/server.go` around lines 95 - 99, The current type assertion for namespaced storage (s.storage.(storage.NamespacedStorage)) calls s.logger.Fatal when it fails during route setup; instead, move this validation to startup (e.g., in main) or convert it to a recoverable error path: perform the assertion earlier (before server.New or before RegisterRoutes) and return an error from the initialization function (or disable Bazel endpoints) rather than calling s.logger.Fatal in the route/setup code; update the code that constructs the server (the place that instantiates s and calls its route registration) to handle the returned error and log/exit in main so the check is a startup validation, referencing the namespaced type storage.NamespacedStorage, the s.storage field, and the s.logger.Fatal call to locate the change.src/internal/handler/bazel_metrics.go (1)
39-44: Consider adding unit specification to the histogram.The
entry_sizehistogram description mentions "bytes" but the metric lacks an explicit unit. OpenTelemetry best practices recommend specifying units for clarity in metric backends.♻️ Add unit specification
entrySize, err := meter.Float64Histogram( "bazel_cache.entry_size", - metric.WithDescription("Size of Bazel cache entries in bytes")) + metric.WithDescription("Size of Bazel cache entries"), + metric.WithUnit("By"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/internal/handler/bazel_metrics.go` around lines 39 - 44, The histogram for "bazel_cache.entry_size" is missing an explicit unit; update the Float64Histogram call (where meter.Float64Histogram is invoked for "bazel_cache.entry_size" with metric.WithDescription) to include a unit option (use metric.WithUnit(unit.Bytes) from the otel unit package or equivalent) so the metric description and backend explicitly know the values are in bytes; ensure you add the necessary import for the unit package.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/internal/handler/bazel_handler.go`:
- Around line 21-34: NewBazelHandler currently accepts negative maxEntrySize
which causes runtime failures; update NewBazelHandler to validate the
maxEntrySize parameter (the constructor for BazelHandler) and return a non-nil
error if maxEntrySize is negative, e.g., check maxEntrySize < 0 at the start of
NewBazelHandler and return a clear error explaining the invalid maxEntrySize
instead of constructing and returning a BazelHandler instance.
In `@src/internal/handler/bazel_put.go`:
- Around line 49-81: The current handler fully buffers uploads into memory
(using limitedReader + io.ReadAll + bytes.NewReader) before calling store.Put;
change it to stream directly to store.Put when verifyHash is false and to
stream-to-temp-file (or spooled buffer) while hashing when verifyHash is true.
Concretely: for the non-verifying path, pass an io.LimitReader (respecting
h.maxEntrySize) or the request body directly to store.Put instead of reading
into data and remove bytes.NewReader usage; for the verifying path, pipe the
request through a hasher (sha256.New) while writing to a temporary file or
spooled buffer (os.CreateTemp or an in-memory spool) so you can compute
computedHex, enforce h.maxEntrySize, record contentLength after write, and then
call store.Put with the temp file reader; keep the existing metrics and error
handling (h.metrics.EntrySize.Record, h.metrics.HashMismatches, h.logger) but
update them to use actual contentLength determined from the stream or temp file,
and ensure you detect oversized uploads by limiting the reader and returning
StatusRequestEntityTooLarge if the limit is exceeded.
---
Nitpick comments:
In `@src/internal/handler/bazel_metrics.go`:
- Around line 39-44: The histogram for "bazel_cache.entry_size" is missing an
explicit unit; update the Float64Histogram call (where meter.Float64Histogram is
invoked for "bazel_cache.entry_size" with metric.WithDescription) to include a
unit option (use metric.WithUnit(unit.Bytes) from the otel unit package or
equivalent) so the metric description and backend explicitly know the values are
in bytes; ensure you add the necessary import for the unit package.
In `@src/internal/server/server.go`:
- Around line 95-99: The current type assertion for namespaced storage
(s.storage.(storage.NamespacedStorage)) calls s.logger.Fatal when it fails
during route setup; instead, move this validation to startup (e.g., in main) or
convert it to a recoverable error path: perform the assertion earlier (before
server.New or before RegisterRoutes) and return an error from the initialization
function (or disable Bazel endpoints) rather than calling s.logger.Fatal in the
route/setup code; update the code that constructs the server (the place that
instantiates s and calls its route registration) to handle the returned error
and log/exit in main so the check is a startup validation, referencing the
namespaced type storage.NamespacedStorage, the s.storage field, and the
s.logger.Fatal call to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f28a846-1863-407f-b130-c4ed4160cc22
📒 Files selected for processing (11)
chart/Chart.yamlchart/templates/configmap.yamlchart/values.yamlsrc/configs/config.yamlsrc/internal/config/config.gosrc/internal/handler/bazel_get.gosrc/internal/handler/bazel_handler.gosrc/internal/handler/bazel_metrics.gosrc/internal/handler/bazel_put.gosrc/internal/handler/validation.gosrc/internal/server/server.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/internal/handler/bazel_put.go (1)
60-71: Metrics may not reflect actual bytes stored when Content-Length is inaccurate.Line 63 records metrics based on the claimed
contentLengthbefore storage completes. If a client sends fewer bytes than declared (connection drop, bug), the metric will overcount. Consider recording metrics based on actual bytes written, similar to the spooling path.This is low-risk for well-behaved Bazel clients, but could affect observability accuracy.
♻️ Possible improvement: use a counting reader
if contentLength >= 0 { // Content-Length known: stream directly to storage - limited := io.LimitReader(c.Request.Body, contentLength) - h.metrics.EntrySize.Record(c.Request.Context(), float64(contentLength), attrs) + countingReader := &countingReader{r: io.LimitReader(c.Request.Body, contentLength)} - if err := store.Put(c.Request.Context(), hash, limited, contentLength); err != nil { + if err := store.Put(c.Request.Context(), hash, countingReader, contentLength); err != nil { h.logger.Error().Err(err).Str("hash", hash).Str("cache_type", cacheType).Msg("failed to store bazel cache entry") c.Status(http.StatusInternalServerError) return } + h.metrics.EntrySize.Record(c.Request.Context(), float64(countingReader.n), attrs) c.Status(http.StatusOK) return }You would need a simple counting wrapper:
type countingReader struct { r io.Reader n int64 } func (c *countingReader) Read(p []byte) (int, error) { n, err := c.r.Read(p) c.n += int64(n) return n, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/internal/handler/bazel_put.go` around lines 60 - 71, The metric is recorded using the declared contentLength before store.Put completes, which can overcount if fewer bytes are actually sent; wrap the limited reader in a simple counting reader (e.g., countingReader that implements io.Reader and tracks bytes read) and pass that to store.Put instead of limited, then after store.Put returns (on success or error) record the actual bytes read with h.metrics.EntrySize.Record(float64(countingReader.n), attrs); update the code paths that currently use io.LimitReader -> limited to use io.LimitReader wrapped by your countingReader and ensure you still handle the existing error logging via h.logger and HTTP response the same way.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/internal/handler/bazel_put.go`:
- Around line 60-71: The metric is recorded using the declared contentLength
before store.Put completes, which can overcount if fewer bytes are actually
sent; wrap the limited reader in a simple counting reader (e.g., countingReader
that implements io.Reader and tracks bytes read) and pass that to store.Put
instead of limited, then after store.Put returns (on success or error) record
the actual bytes read with h.metrics.EntrySize.Record(float64(countingReader.n),
attrs); update the code paths that currently use io.LimitReader -> limited to
use io.LimitReader wrapped by your countingReader and ensure you still handle
the existing error logging via h.logger and HTTP response the same way.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e5bbdff-120e-4dad-b0f1-3d439680cd4d
📒 Files selected for processing (1)
src/internal/handler/bazel_put.go
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
1 participant
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.
Summary by CodeRabbit
New Features
Bug Fixes
Chores