feat(storage): Implement Tiered Object Storage with S3/R2 Support #150
Conversation
Signed-off-by: jackie <[email protected]>
Signed-off-by: jackieismpc <[email protected]>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b7a7301b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR introduces an async-aware, pluggable storage architecture with a new Storage trait, local/remote/tiered backends, and R2/S3 support via object_store, while keeping the existing synchronous ClientStorage API by bridging through an internal Tokio runtime. It also wires in the necessary Buck/Cargo dependencies and adds tests for the new storage behavior, including in-memory and Cloudflare R2 integration tests.
Changes:
- Added
Storagetrait plusLocalStorage,RemoteStorage(viaobject_store), andTieredStorage(local+remote with size-based LRU caching) undersrc/utils/storage/. - Refactored
ClientStorageto delegate to an asyncStoragebackend through a dedicated Tokio runtime and environment-driven backend selection (LIBRA_STORAGE_*). - Extended build metadata (Cargo, BUCK, third-party crates) and added tests for remote/tiered behavior and R2 integration, plus a small fix to
index_packtemp-dir usage.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
third-party/rust/crates/ring/0.17.14/BUCK |
Enables the std feature for ring to satisfy TLS requirements from object_store. |
third-party/rust/crates/quick-xml/0.38.4/BUCK |
Adds Buck rule for quick-xml, used transitively by object_store. |
third-party/rust/crates/object_store/0.12.4/BUCK |
Adds Buck rule for object_store with cloud/back-end features needed for S3/R2-style storage. |
third-party/rust/crates/humantime/2.3.0/BUCK |
Adds Buck rule for humantime, a dependency of object_store. |
tests/storage_r2_test.rs |
New tests exercising RemoteStorage with in-memory backend, TieredStorage size-based behavior, and (ignored-by-default) Cloudflare R2 integration using ClientStorage. |
src/utils/storage/tiered.rs |
Implements TieredStorage that routes reads/writes between LocalStorage and RemoteStorage, caching large objects locally in an LRU that deletes files on eviction. |
src/utils/storage/remote.rs |
Implements RemoteStorage using object_store::ObjectStore, handling Git object header construction, zlib compression, and path mapping. |
src/utils/storage/mod.rs |
Defines the async Storage trait (get/put/exist/search) and re-exports the new storage backends. |
src/utils/storage/local.rs |
Refactors and encapsulates the previous on-disk Git object storage logic (loose + pack, including index building and caches) behind the Storage trait, adding hash-kind capture for multi-threaded contexts. |
src/utils/mod.rs |
Exposes the new storage module from utils. |
src/utils/client_storage.rs |
Reworks ClientStorage into a thin synchronous facade over async Storage implementations, introduces a dedicated Tokio runtime, environment-based backend selection, and adjusts tests accordingly. |
src/command/index_pack.rs |
Changes pack index v2 building to use the system temp directory for Pack scratch space to avoid DirectoryNotEmpty errors during cleanup. |
buckal.snap |
Updates Buckal fingerprints to account for new dependencies (object_store, quick-xml, humantime, modified ring features). |
Cargo.toml |
Adds object_store and async-trait dependencies to the libra crate. |
Cargo.lock |
Locks in versions for object_store, quick-xml, humantime, and updates dependency graph accordingly. |
BUCK |
Wires async-trait and object_store into the main binary/library/test targets and adds a Buck target for the new storage_r2_test. |
Signed-off-by: jackie <[email protected]>
| .inner | ||
| .get(&path) | ||
| .await | ||
| .map_err(|e| GitError::ObjectNotFound(format!("Remote object not found: {}", e)))?; |
There was a problem hiding this comment.
In RemoteStorage::get, all errors from inner.get(&path).await are mapped to GitError::ObjectNotFound, which conflates genuine "not found" cases with network, authentication, or other backend failures and makes debugging operational issues much harder. Consider preserving the underlying error type (e.g., using a dedicated GitError variant or attaching the original error as context) so callers can distinguish between missing objects and remote I/O/configuration problems.
| .map_err(|e| GitError::ObjectNotFound(format!("Remote object not found: {}", e)))?; | |
| .map_err(|e| match e { | |
| object_store::Error::NotFound { .. } => { | |
| GitError::ObjectNotFound(format!("Remote object not found at path {}", path)) | |
| } | |
| other => GitError::IOError(std::io::Error::other(other)), | |
| })?; |
| unsafe { | ||
| env::set_var("LIBRA_STORAGE_TYPE", "r2"); | ||
| env::set_var("LIBRA_STORAGE_BUCKET", "libra"); // Non-existent bucket (initially) | ||
| env::set_var( | ||
| "LIBRA_STORAGE_ENDPOINT", | ||
| format!("https://{}.r2.cloudflarestorage.com", R2_ACCOUNT_ID), | ||
| ); | ||
| env::set_var("LIBRA_STORAGE_ACCESS_KEY", R2_ACCESS_KEY); | ||
| env::set_var("LIBRA_STORAGE_SECRET_KEY", R2_SECRET_KEY); | ||
| env::set_var("LIBRA_STORAGE_REGION", "auto"); | ||
| env::set_var("LIBRA_STORAGE_THRESHOLD", "10"); | ||
| } |
There was a problem hiding this comment.
These integration tests wrap a series of env::set_var calls in an unsafe block even though set_var is safe, which is misleading and may suggest that undefined behavior is involved where there is none. You can drop the unsafe block entirely and call env::set_var directly, keeping the tests clearer and avoiding unnecessary use of unsafe.
| unsafe { | |
| env::set_var("LIBRA_STORAGE_TYPE", "r2"); | |
| env::set_var("LIBRA_STORAGE_BUCKET", "libra"); // Non-existent bucket (initially) | |
| env::set_var( | |
| "LIBRA_STORAGE_ENDPOINT", | |
| format!("https://{}.r2.cloudflarestorage.com", R2_ACCOUNT_ID), | |
| ); | |
| env::set_var("LIBRA_STORAGE_ACCESS_KEY", R2_ACCESS_KEY); | |
| env::set_var("LIBRA_STORAGE_SECRET_KEY", R2_SECRET_KEY); | |
| env::set_var("LIBRA_STORAGE_REGION", "auto"); | |
| env::set_var("LIBRA_STORAGE_THRESHOLD", "10"); | |
| } | |
| env::set_var("LIBRA_STORAGE_TYPE", "r2"); | |
| env::set_var("LIBRA_STORAGE_BUCKET", "libra"); // Non-existent bucket (initially) | |
| env::set_var( | |
| "LIBRA_STORAGE_ENDPOINT", | |
| format!("https://{}.r2.cloudflarestorage.com", R2_ACCOUNT_ID), | |
| ); | |
| env::set_var("LIBRA_STORAGE_ACCESS_KEY", R2_ACCESS_KEY); | |
| env::set_var("LIBRA_STORAGE_SECRET_KEY", R2_SECRET_KEY); | |
| env::set_var("LIBRA_STORAGE_REGION", "auto"); | |
| env::set_var("LIBRA_STORAGE_THRESHOLD", "10"); |
|
Claude finished @genedna's task in 2m 23s —— View job Code Review CompleteI've completed a comprehensive code review of this PR introducing tiered object storage with S3/R2 support. This is a significant architectural improvement that adds multi-cloud storage capabilities while maintaining backward compatibility. Progress
SummaryThis is a well-structured refactoring that introduces a clean storage abstraction layer. The implementation demonstrates good architectural decisions with the async trait pattern and tiered storage approach. However, there are several critical issues and improvements needed before merging. Critical Issues 🔴1. LRU Cache Size Accounting Bug (src/utils/storage/tiered.rs:22-24)The automated review correctly identified this. Problem: The cache tracks file sizes correctly (line 95: Impact: Medium - The code works as intended for disk cache management, but the semantic mismatch could cause confusion. Recommendation: Either rename 2. Remote Search Not Implemented (src/utils/storage/remote.rs:104-137)The bot reviews identified that Issue: Impact: High - Abbreviated object IDs and ref navigation (e.g., Test Coverage: The test at Recommendation: In 3. Panic on Invalid Storage Type (src/utils/client_storage.rs:73)_ => panic!("Unsupported storage type: {}", storage_type),Problem: Configuration errors cause hard crashes instead of graceful error handling. Impact: High - Production misconfiguration leads to panics rather than recoverable errors. Recommendation: Return a Result or fall back to local storage with a warning. The bot review correctly identified this. Fix this → 4. Error Conflation in Remote Storage (src/utils/storage/remote.rs:41).map_err(|e| GitError::ObjectNotFound(format!("Remote object not found: {}", e)))?;Problem: All errors (network failures, auth errors, timeouts, actual 404s) are mapped to Impact: High - Debugging operational issues becomes very difficult when all failures look like missing objects. Recommendation: Add error context preservation. Consider the suggestion from the bot review to match on High Priority Issues 🟡5. Unnecessary Unsafe Blocks (tests/storage_r2_test.rs:130, 175)unsafe {
env::set_var("LIBRA_STORAGE_TYPE", "r2");
// ...
}Problem: Impact: Medium - Code clarity and false sense of danger around safe operations. Note: Multiple bot reviews flagged this consistently. Recommendation: Remove all 6. Hardcoded Test Credentials (tests/storage_r2_test.rs:13-15)const R2_ACCOUNT_ID: &str = "1234";
const R2_ACCESS_KEY: &str = "12345678";
const R2_SECRET_KEY: &str = "dbedudygydgwedqudgwedqugwedqugwedqugwedqugwedqugwedqugwedqugwedqug";Problem: Even though these are dummy values, having credential patterns in source code is a security anti-pattern. Impact: Medium - Could lead to accidental credential commits in the future. Recommendation: Load from environment variables or use clearly fake values like 7. Hash Kind Thread Safety (src/utils/storage/local.rs:52, 387-389)hash_kind: Option<HashKind>, // Capture hash kind from creation thread
// ...
if let Some(kind) = self_clone.hash_kind {
set_hash_kind(kind);
}Problem: The code captures Impact: Medium - Potential race conditions in multi-threaded scenarios with mixed hash types. Architecture: The PR description mentions this fixes "HashKind context loss issues in multi-threaded environments," which is good. However, relying on thread-local state is brittle. Recommendation: Consider making 8. Index Pack Temp Directory Change (src/command/index_pack.rs:200-201)// Use system temp dir to avoid DirectoryNotEmpty error if Pack tries to clean up the dir
let tmp_path = std::env::temp_dir();Problem: This changes behavior to use system temp instead of pack directory. While it fixes the
Impact: Medium - Could affect reliability and performance. Recommendation: Document why this change was needed and ensure proper cleanup. Consider using Medium Priority Issues 🟢9. Missing Documentation for Storage TraitThe
10. No Validation of Environment VariablesThe storage configuration (src/utils/client_storage.rs:46-90) reads environment variables but doesn't validate them:
Recommendation: Add basic validation and return clear error messages. 11. Blocking Operations in Async ContextThe let (tx, rx) = mpsc::channel();
RUNTIME.spawn(async move {
let res = future.await;
let _ = tx.send(res);
});
rx.recv().unwrap()Issue: Each storage operation spawns a task and uses a channel. For high-frequency operations, this could be expensive. Recommendation: Consider using Security Considerations 🔒12. HTTP Allowed by DefaultLine 69: Problem: This allows unencrypted HTTP connections to object storage, which could expose credentials and data. Impact: High if used in production, Low if only for local testing. Recommendation: Make this configurable via environment variable and default to 13. No Credential ValidationThe code accepts any credential values without validation. Consider:
Test Coverage Assessment ✅Strengths:
Gaps:
Recommendation: Add negative test cases and stress tests for cache eviction. Code Quality 👍Positives:
Areas for Improvement:
Performance Considerations ⚡
Documentation 📚The PR description is excellent and clearly explains the changes. The code-level documentation is good in implementation files but sparse in the trait definition. Consider adding:
Overall AssessmentThis is a well-architected PR that makes meaningful improvements to Libra's storage layer. The core design is sound, and the implementation is mostly clean. However, the critical issues around error handling, search functionality in tiered mode, and configuration validation should be addressed before merging. Recommendation: Approve with Changes RequiredThe automated bot reviews correctly identified several issues. My additional findings focus on architectural concerns and subtle bugs that could cause production issues. Must Fix Before Merge:
Should Fix Before Merge:
Can Address in Follow-up:
Great work on this significant refactoring! The tiered storage architecture is a solid foundation for multi-cloud support. 🎉 |
2 participants
Summary:
This PR refactors the underlying storage module of Libra, introducing a multi-cloud adapter based on
object_storeand an intelligent tiered storage architecture. By using a "Synchronous Facade" pattern to encapsulate underlying asynchronous I/O, it achieves seamless support for S3/R2 without breaking existing business logic.Key Changes:
Dependencies:
object_store,async-trait,lru-mem, andtokio.Storage Architecture Refactor (
src/utils/storage/):StorageTrait: Defined standard async storage interfaces (get,put,exist,search).LocalStorage: Migrated and optimized existing file system storage logic, resolvingHashKindcontext loss issues in multi-threaded environments.RemoteStorage: Implemented object access based on S3/R2, handling Git object compression and header encapsulation.TieredStorage: Implemented tiered routing logic and LRU cache management.Client Adaptation (
src/utils/client_storage.rs):Refactored
ClientStorageto maintain an internaltokio::runtime::Runtime, bridging synchronous API calls to underlying asynchronous implementations.Supported dynamic storage mode switching via environment variables (
LIBRA_STORAGE_TYPE,LIBRA_STORAGE_BUCKET, etc.).Tests:
Added
tests/storage_r2_test.rs: Includes Mock backend tests and real R2 environment integration tests.Fixed concurrency test bugs in
index_pack.rsandclient_storage.rs.引入多云对象存储支持与智能分层存储策略 #149