feat(cas): add node-local content-addressable store for model blobs#38
Closed
chlins wants to merge 1 commit into
Closed
feat(cas): add node-local content-addressable store for model blobs#38chlins wants to merge 1 commit into
chlins wants to merge 1 commit into
Conversation
📊 Code Coverage Report
📦 Per-package breakdown |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request upgrades the project to Go 1.25, updates several dependencies, and introduces a node-local content-addressable store (CAS) for model layer blobs. The CAS implementation uses hardlinks for deduplication and reference counting to manage blob lifecycles. Integration hooks are added to the pull process to short-circuit downloads on cache hits. Review feedback recommends using standard library functions for hex validation, handling ignored errors during blob import and reference release, and utilizing error aggregation when releasing multiple layers.
Comment on lines
+101
to
+106
| for i := 0; i < len(trimmed); i++ { | ||
| c := trimmed[i] | ||
| if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F')) { | ||
| return "" | ||
| } | ||
| } |
There was a problem hiding this comment.
The manual validation for the hex-encoded digest can be simplified and made more robust by using hex.DecodeString from the standard library. This avoids manual character checking and is more idiomatic.
if _, err := hex.DecodeString(trimmed); err != nil {
return ""
}
Comment on lines
+199
to
+200
| _ = s.ReleaseLayer(context.Background(), digest, ownerKey) | ||
| return false, ctx.Err() |
There was a problem hiding this comment.
The error from s.ReleaseLayer is ignored. If releasing the layer fails, the reference will be leaked, which contradicts the intention of the comment on line 198. This error should be handled, for example by logging it or wrapping it with the context error, to aid in debugging.
if releaseErr := s.ReleaseLayer(context.Background(), digest, ownerKey); releaseErr != nil {
return false, errors.Wrapf(ctx.Err(), "while also failing to release ref: %v", releaseErr)
}
return false, ctx.Err()
Comment on lines
+354
to
+371
| var firstErr error | ||
| for _, e := range entries { | ||
| name := e.Name() | ||
| if !strings.HasSuffix(name, ".refs") { | ||
| continue | ||
| } | ||
| digest := decodeDigestFromRefsName(name) | ||
| if digest == "" { | ||
| continue | ||
| } | ||
| if _, err := os.Stat(filepath.Join(blobsRoot(s.root), name, ownerKey)); err != nil { | ||
| continue | ||
| } | ||
| if err := s.ReleaseLayer(ctx, digest, ownerKey); err != nil && firstErr == nil { | ||
| firstErr = err | ||
| } | ||
| } | ||
| return firstErr |
There was a problem hiding this comment.
This function only returns the first error encountered during the loop. If multiple calls to ReleaseLayer fail, all but the first error will be silently ignored. It would be more robust to collect all errors and return them, for example by using errors.Join (available in Go 1.20+).
Suggested change
| var firstErr error | |
| for _, e := range entries { | |
| name := e.Name() | |
| if !strings.HasSuffix(name, ".refs") { | |
| continue | |
| } | |
| digest := decodeDigestFromRefsName(name) | |
| if digest == "" { | |
| continue | |
| } | |
| if _, err := os.Stat(filepath.Join(blobsRoot(s.root), name, ownerKey)); err != nil { | |
| continue | |
| } | |
| if err := s.ReleaseLayer(ctx, digest, ownerKey); err != nil && firstErr == nil { | |
| firstErr = err | |
| } | |
| } | |
| return firstErr | |
| var errs []error | |
| for _, e := range entries { | |
| name := e.Name() | |
| if !strings.HasSuffix(name, ".refs") { | |
| continue | |
| } | |
| digest := decodeDigestFromRefsName(name) | |
| if digest == "" { | |
| continue | |
| } | |
| if _, err := os.Stat(filepath.Join(blobsRoot(s.root), name, ownerKey)); err != nil { | |
| continue | |
| } | |
| if err := s.ReleaseLayer(ctx, digest, ownerKey); err != nil { | |
| errs = append(errs, err) | |
| } | |
| } | |
| return errors.Join(errs...) |
| // Best-effort: an Import failure leaves the extracted file in place, | ||
| // so the volume still works; we just miss the cache opportunity. | ||
| // Import wakes followers internally on both paths. | ||
| _ = h.store.Import(h.ctx, desc.Digest.String(), dst) |
There was a problem hiding this comment.
The error from h.store.Import is ignored. While the comment states this is a best-effort operation, silently failing to import a blob into the cache can lead to missed optimization opportunities and makes debugging difficult. It would be beneficial to log this error to provide visibility into why a blob might not be cached.
chlins
force-pushed
the
refactor/storage
branch
4 times, most recently
from
ae66783 to
dfdee47
Compare
Signed-off-by: chlins <chlins.zhang@gmail.com>
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.
This pull request introduces a new content-addressable storage (CAS) implementation for model layer blobs and upgrades the project to Go 1.25, along with a broad set of dependency updates. The new
pkg/cas/cas.gofile provides a robust, deduplicated, and reference-counted local blob store with concurrency-safe download deduplication. The Go version bump is reflected across the build, CI, and module files, and several dependencies are updated to more recent versions for improved compatibility and security.Major features and improvements:
1. New Content-Addressable Store Implementation
pkg/cas/cas.go, a new package implementing a local CAS for model layer blobs. Features include deduplication via hardlinks, per-owner reference counting, concurrency-safe download deduplication, and atomic publishing of blobs. The API supports claiming references, importing blobs, and releasing references, with careful handling of concurrent downloads and cleanup of stale temporary files.2. Go Version Upgrade
go.mod, the Docker build file, and the GitHub Actions workflow for coverage. This ensures the project uses the latest language features and improvements. [1] [2] [3]3. Dependency Updates
go.modto newer versions, including major libraries such asgo-git,modctl,go.opentelemetry.io/otel,google.golang.org/grpc, and others. These updates improve security, compatibility, and performance. [1] [2] [3] [4]4. Minor CI Cleanup