Skip to content

refactor(storage): carry the cache hit through open instead of unwrapping it - #567

Merged
BryanFRD merged 2 commits into
mainfrom
refactor/backend-cache-unwrap
Oct 10, 2026
Merged

BryanFRD merged 2 commits into
mainfrom
refactor/backend-cache-unwrap

Conversation

@BryanFRD

Copy link
Copy Markdown
Contributor

Closes #566

Store::open for a bucket now returns the cache alongside the reader when the reader came from it ((reader, hit)), and the raw-object branch reopens through hit. That removes the from_cache flag and the cache.as_ref().unwrap() behind it, and the two identical Object::Remote arms collapse into one. The invariant is now in the types.

No behaviour change. I added two tests that run against the in-process bucket stub, so they need no emulator:

  • a raw object is read from the bucket cold (Object::Remote), the cache is filled behind the read, and the next read is Object::Raw with the same bytes;
  • with no cache configured, a raw object is always Object::Remote.

Both also pass against the code on main, which is the point: they pin the behaviour the refactor must keep. The only existing test of this branch, a_cached_download_is_the_same_download, needs a bucket emulator and only runs in CI.

@BryanFRD
BryanFRD enabled auto-merge (squash) October 10, 2026 15:22

@ferrfleet ferrfleet Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The refactor doesn't change behaviour: hit is Some only when the cache served the reader, and that's the condition from_cache used to check, so the unwrap goes away with nothing else changing. The new warm-cache test is sound. Cache::fill writes the .b3 digest before it renames the object into place, so polling for the object file can't race the integrity check in open.

One formatting nit inline.

Comment on lines +299 to +302
assert!(
copied.exists(),
"the copy is filled in behind the first read"
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: this fits within the default 100-column width, so cargo fmt --check will want it on one line.

Suggested change
assert!(
copied.exists(),
"the copy is filled in behind the first read"
);
assert!(copied.exists(), "the copy is filled in behind the first read");

@BryanFRD
BryanFRD merged commit d63753e into main Oct 10, 2026
29 checks passed
@BryanFRD
BryanFRD deleted the refactor/backend-cache-unwrap branch October 10, 2026 15:59
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.

Bucket open unwraps the cache behind a flag set earlier

1 participant