Skip to content

fix: getting deleted blobs should return error #95

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Frando
Copy link
Member

@Frando Frando commented Jul 3, 2025

Description

Right now there is a bug in the store: When you delete a blob by deleting its tag, run gc, and then do store.get_bytes(hash), it will not return an error but instead an emtpy byte slice (b""). This, of course, is very wrong: we should never return data that doesn't match the requested hash.

This PR fixes the logic to not create empty blobs on-demand when running get or export operations.

Also adds a test that ensures that, for deleted blobs, errors are returned for get_bytes, export, export_bao, export_ranges for both the fs and mem store.

Additionally, I noticed that Blobs::delete and delete_with_opts is not actually usable as a public API because there's internal protections of hashes that make it not work as intended (it is only used from gc, where protections are cleared before). Therefore I made the function non-public to not confuse users.

Breaking Changes

  • iroh_blobs::api::blobs::Blobs::delete and delete_with_opts are no longer public. These functions did not actually delete blobs, because blobs would usually be protected. The function is now only used internally during garbage collection (where protections are cleared beforehand in a non-public way). Public users can only use garbage collection for deletion of blobs.

Notes & open questions

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.

@Frando Frando changed the title fix: getting deleted blobs returns errors fix: getting deleted blobs should return errors Jul 3, 2025
@Frando Frando changed the title fix: getting deleted blobs should return errors fix: getting deleted blobs should return error Jul 3, 2025
Copy link

github-actions bot commented Jul 3, 2025

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh-blobs/pr/95/docs/iroh_blobs/

Last updated: 2025-07-03T10:37:47Z

@Frando Frando mentioned this pull request Jul 3, 2025
4 tasks
@n0bot n0bot bot added this to iroh Jul 3, 2025
@github-project-automation github-project-automation bot moved this to 🏗 In progress in iroh Jul 3, 2025
@Frando
Copy link
Member Author

Frando commented Jul 3, 2025

My changes in this commit ea29863 made other tests fail. To make them pass again, I pushed this commit dd59017.

It does this:

  • Add an option for export_bao to optionally create the entry if it doesn't exist
  • Port the special-case handling for Hash::EMPTY from the fs store to the mem store

With these two changes, all tests pass again.

Not too sure about the special casing of the empty hash - I'd have to dive in deeper to analyze this, but seems fine because the fs store does it too?

instead handle the error in remote.
@rklaehn
Copy link
Collaborator

rklaehn commented Jul 3, 2025

This is quite an embarrassing bug. Thanks for finding this first.

I don't think the create_if_missing parameter is useful. I removed it again and instead handled the error in the code where it computes local data for a HashSeq. WDYT?

@rklaehn
Copy link
Collaborator

rklaehn commented Jul 4, 2025

Not too sure about the special casing of the empty hash - I'd have to dive in deeper to analyze this, but seems fine because the fs store does it too?

The empty hash is weird. Also, I don't want to remember whether I have the empty hash or not, and it costs nothing to always have it. That's the reason for the special case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

2 participants