Prefer `VersionStore::store_version_from_reader` by CleanCut · Pull Request #405 · Oxen-AI/Oxen

CleanCut · 2026-03-30T23:56:41Z

This PR removes VersionStore::store_version_from_path (and its implementations) in favor of using VersionStore::store_version_from_reader. This makes it more straightforward to maintain the VersionStore trait and its various implementations, and makes it clearer how file bytes are traveling through the various calls.

…producer task errors out--otherwise we may attempt to begin or continue uploads after sending abort_multipart_upload, which would be weird

coderabbitai · 2026-03-30T23:56:47Z

📝 Walkthrough

Summary by CodeRabbit

Documentation
- Added repository guidelines for async IO in Rust code.
- Added "Large File Support" roadmap with planned implementation phases.
Refactor
- Refactored file storage operations to use async reader-based approach.
- Standardized file ingestion across multiple storage operations for improved scalability.

Walkthrough

This PR removes the synchronous path-based store_version_from_path API from the VersionStore trait and migrates all call sites to use the async reader-based store_version_from_reader method instead. Files are now read asynchronously, with content passed via readers and explicit size parameters, aligning with the codebase's shift to async IO practices.

Changes

Cohort / File(s)	Summary
Documentation Updates `.claude/CLAUDE.md`, `TODO.md`	Added async IO guideline for Rust code and new "Large File Support" section describing a three-phase plan for handling large files with streaming and chunking.
Call Site Migrations `crates/lib/src/api/client/entries.rs`, `crates/lib/src/core/v_latest/add.rs`, `crates/lib/src/core/v_latest/branches.rs`, `crates/lib/src/core/v_latest/workspaces/files.rs`, `crates/lib/src/repositories/remote_mode/checkout.rs`	Updated version storage calls from `store_version_from_path(hash, path)` to async reader-based `store_version_from_reader(hash, Box::new(reader), size)`, including async file opening and metadata retrieval at each call site.
API Removal `crates/lib/src/storage/version_store.rs`, `crates/lib/src/storage/local.rs`, `crates/lib/src/storage/s3.rs`	Removed `store_version_from_path` method declaration from the VersionStore trait and its implementations in LocalVersionStore and S3VersionStore. Removed sync file-loading logic from S3 implementation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Implement S3VersionStore::store_version_from_reader #398: Implements and modifies S3VersionStore::store_version_from_reader signature and streaming behavior, directly related to the reader-based API now being adopted across call sites.
Tests for S3VersionStore::store_version_from_reader #404: Adds test helpers and implements store_version_from_reader for S3VersionStore, directly paired with this PR's migration of callers to that method.
Oxen-AI/Oxen#647: Modifies process_add_dir in the same file (core/v_latest/add.rs) to refactor add logic, creating potential conflicts or dependencies with these storage call changes.

Suggested reviewers

malcolmgreaves
jcelliott
rpschoenburg

Poem

🐰 A hop from paths to readers swift,
Async files on Tokio's lift,
No blocking here, just streams that flow,
The storage layer steals the show! 📚✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly and precisely describes the main change: removing `store_version_from_path` in favor of `store_version_from_reader`.
Description check	✅ Passed	The description accurately explains the change and its rationale, detailing both the removal of the old method and the benefits of using the reader-based approach.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch cleancut/rm-store-version-from-path

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

…ror that didn't originate from the aws sdk

…t multipart upload shenanigans

…3 without multipart upload shenanigans" because it fails at runtime without a size This reverts commit 7690d04.

…ge in the direct streaming attempt

…t; use file size to determine part size for multipart uploads for files > 100MB

Co-authored-by: Malcolm Greaves <malcolmgreaves@users.noreply.github.com>

…ires updating some dependencies

…in favor of using VersionStore::store_version_from_reader

coderabbitai

🧹 Nitpick comments (1)

crates/lib/src/api/client/entries.rs (1)
242-249: Consider streaming from disk instead of re-buffering in memory.

The current implementation reads the entire file into memory with tokio::fs::read, computes the hash, then passes the same bytes via Cursor to store_version_from_reader. For large files, this keeps the full content in memory.

Since the hash computation requires the full content, the memory usage isn't a regression from before. However, for consistency with other call sites and better memory efficiency on large files, consider refactoring to:

Compute hash during download (streaming hash)

Then stream from disk to version store using tokio::fs::File + BufReader

This would avoid holding the full file in memory after hashing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/lib/src/api/client/entries.rs` around lines 242 - 249, The code
currently reads the whole file into memory via tokio::fs::read, computes the
hash with util::hasher::hash_buffer and then constructs a Cursor to call
version_store.store_version_from_reader, which keeps the full buffer in memory;
change this to compute the hash while streaming (e.g., open the file with
tokio::fs::File and feed bytes through a streaming hasher during download or
read), then reopen or seek the file and pass a tokio::fs::File wrapped in
tokio::io::BufReader (or an async Read) into
version_store.store_version_from_reader along with the computed hash and size so
the full content is not re-buffered in memory.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@crates/lib/src/api/client/entries.rs`:
- Around line 242-249: The code currently reads the whole file into memory via
tokio::fs::read, computes the hash with util::hasher::hash_buffer and then
constructs a Cursor to call version_store.store_version_from_reader, which keeps
the full buffer in memory; change this to compute the hash while streaming
(e.g., open the file with tokio::fs::File and feed bytes through a streaming
hasher during download or read), then reopen or seek the file and pass a
tokio::fs::File wrapped in tokio::io::BufReader (or an async Read) into
version_store.store_version_from_reader along with the computed hash and size so
the full content is not re-buffered in memory.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6bd41b51-0e02-40c9-8f84-314469b879ad

📥 Commits

Reviewing files that changed from the base of the PR and between d378f82 and 4328f2e.

📒 Files selected for processing (10)

.claude/CLAUDE.md
TODO.md
crates/lib/src/api/client/entries.rs
crates/lib/src/core/v_latest/add.rs
crates/lib/src/core/v_latest/branches.rs
crates/lib/src/core/v_latest/workspaces/files.rs
crates/lib/src/repositories/remote_mode/checkout.rs
crates/lib/src/storage/local.rs
crates/lib/src/storage/s3.rs
crates/lib/src/storage/version_store.rs

💤 Files with no reviewable changes (3)

crates/lib/src/storage/local.rs
crates/lib/src/storage/s3.rs
crates/lib/src/storage/version_store.rs

CleanCut added 3 commits March 27, 2026 17:03

implement S3VersionStore::store_version_from_reader

bb7603a

refactor to abort the upload task (and any in-flight uploads) if the …

95a2988

…producer task errors out--otherwise we may attempt to begin or continue uploads after sending abort_multipart_upload, which would be weird

resolve merge conflict

0645c40

CleanCut added 6 commits March 31, 2026 10:57

tighten up OxenError::AwsSdkError so we can't accidentally wrap an er…

183f82b

…ror that didn't originate from the aws sdk

upload error instead of basic error

09d859f

refactor S3 store_version_from_reader to stream directly to s3 withou…

7690d04

…t multipart upload shenanigans

Revert "refactor S3 store_version_from_reader to stream directly to s…

5b81191

…3 without multipart upload shenanigans" because it fails at runtime without a size This reverts commit 7690d04.

use multipart upload, but using the owned reader that was a nice chan…

7a97371

…ge in the direct streaming attempt

require specifying the file size; upload files up to 100MB in one sho…

85dded8

…t; use file size to determine part size for multipart uploads for files > 100MB

CleanCut force-pushed the cleancut/s3-tests branch from e689414 to 16d3337 Compare April 1, 2026 03:54

CleanCut force-pushed the cleancut/rm-store-version-from-path branch 2 times, most recently from 510937a to 816bdf6 Compare April 1, 2026 15:36

resolve merge conflict

ed1b10f

CleanCut force-pushed the cleancut/s3-tests branch from 1ac0ab1 to 58091e1 Compare April 1, 2026 15:46

CleanCut force-pushed the cleancut/rm-store-version-from-path branch 2 times, most recently from 792882a to a7c773f Compare April 1, 2026 15:48

CleanCut and others added 5 commits April 1, 2026 11:05

Apply suggestions from code review

e6ac7a1

Co-authored-by: Malcolm Greaves <malcolmgreaves@users.noreply.github.com>

address review comment

7907198

write tests for S3VersionStore::store_version_from_reader, which requ…

9299cf1

…ires updating some dependencies

move all dependencies to the workspace

47ea7c0

Cargo.lock just wants to update

f2c6737

CleanCut force-pushed the cleancut/s3-tests branch from 6cc41e0 to f2c6737 Compare April 1, 2026 17:13

remove VersionStore::store_version_from_path and its implementations …

20dd721

…in favor of using VersionStore::store_version_from_reader

CleanCut force-pushed the cleancut/rm-store-version-from-path branch from a7c773f to 20dd721 Compare April 1, 2026 17:15

Base automatically changed from cleancut/s3-tests to main April 2, 2026 02:42

resolve merge conflict

4328f2e

coderabbitai bot reviewed Apr 2, 2026

View reviewed changes

malcolmgreaves approved these changes Apr 3, 2026

View reviewed changes

CleanCut merged commit bfa09f0 into main Apr 3, 2026
5 checks passed

CleanCut deleted the cleancut/rm-store-version-from-path branch April 3, 2026 20:03

coderabbitai bot mentioned this pull request Apr 3, 2026

Use streaming in S3 version of copy_version_to_path #410

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefer `VersionStore::store_version_from_reader`#405

Prefer `VersionStore::store_version_from_reader`#405
CleanCut merged 17 commits intomainfrom
cleancut/rm-store-version-from-path

CleanCut commented Mar 30, 2026

Uh oh!

coderabbitai bot commented Mar 30, 2026 •

edited

Loading

Summary by CodeRabbit

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

CleanCut commented Mar 30, 2026

Uh oh!

coderabbitai bot commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai bot commented Mar 30, 2026 •

edited

Loading