feat: blob cache manager#606
Open
nogringo wants to merge 4 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR adds a pluggable local blob caching layer to NDK for Blossom file operations. It introduces a ChangesBlob Cache Feature Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/ndk/lib/data_layer/repositories/blob_cache/idb_blob_cache_manager.dart (1)
181-200: 💤 Low valueVerify getTotalSize handles null sizes correctly.
Line 196 only sums
sizewhen it's anint, which correctly handles the new nullable size behavior fromBlobDescriptor. Blobs with unparseable or missing sizes are excluded from the total.Consideration: Document whether this is the intended behavior. Alternatively, you could:
- Count missing sizes as 0 (restore old behavior)
- Track missing sizes separately and log a warning
- Fail fast if any descriptor lacks a size
The current approach (skip nulls) seems reasonable for a cache manager, but explicit documentation would help.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ndk/lib/data_layer/repositories/blob_cache/idb_blob_cache_manager.dart` around lines 181 - 200, getTotalSize currently ignores non-int or missing `size` fields from stored descriptors, which is ambiguous; update the getTotalSize implementation in idb_blob_cache_manager.dart (function getTotalSize, using _kMetadataStore and BlobDescriptor semantics) to treat missing or non-int sizes as 0 and add a clarifying comment above the loop documenting this behavior so totals remain deterministic for cache accounting. Ensure you still only sum sizes when they parse to int (coerce or default to 0) and do not attempt to load payloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ndk/lib/domain_layer/repositories/blob_cache_manager.dart`:
- Around line 19-33: The saveBlob contract currently trusts the caller-provided
sha256 which can enable cache poisoning; update the saveBlob implementation to
compute the SHA-256 of the provided data and compare it to the sha256 parameter
(when given), failing (throwing/returning an error) on mismatch, or
alternatively add an explicit verification flag (e.g., verifySha256) defaulting
to true so callers can opt out for performance; ensure the code path that
updates/returns BlobDescriptor enforces this verification and documents the
behavior for callers of saveBlob and consumers of BlobDescriptor.
In `@packages/ndk/lib/domain_layer/usecases/files/blossom.dart`:
- Around line 342-347: The cache backend exceptions (e.g., from
_blobCache.saveBlob) are currently allowed to abort the primary network flows in
getBlob and deleteBlob; change both code paths so cache failures are non-fatal:
wrap calls to _blobCache.saveBlob (in getBlob) and any _blobCache.delete* calls
(in deleteBlob) in try/catch blocks, log a warning/error on failure, and do not
rethrow so the successful network download or server delete still completes;
ensure the server/network operation remains the primary awaited action and cache
cleanup is best-effort only.
---
Nitpick comments:
In
`@packages/ndk/lib/data_layer/repositories/blob_cache/idb_blob_cache_manager.dart`:
- Around line 181-200: getTotalSize currently ignores non-int or missing `size`
fields from stored descriptors, which is ambiguous; update the getTotalSize
implementation in idb_blob_cache_manager.dart (function getTotalSize, using
_kMetadataStore and BlobDescriptor semantics) to treat missing or non-int sizes
as 0 and add a clarifying comment above the loop documenting this behavior so
totals remain deterministic for cache accounting. Ensure you still only sum
sizes when they parse to int (coerce or default to 0) and do not attempt to load
payloads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0708f4d7-dee0-468d-8345-5d48d429b65e
📒 Files selected for processing (16)
doc/guides/persistence.mddoc/usecases/blossom.mdpackages/ndk/lib/data_layer/repositories/blob_cache/idb_blob_cache_manager.dartpackages/ndk/lib/data_layer/repositories/blob_cache/noop_blob_cache_manager.dartpackages/ndk/lib/domain_layer/entities/blossom_blobs.dartpackages/ndk/lib/domain_layer/repositories/blob_cache_manager.dartpackages/ndk/lib/domain_layer/usecases/files/blossom.dartpackages/ndk/lib/ndk.dartpackages/ndk/lib/presentation_layer/init.dartpackages/ndk/lib/presentation_layer/ndk_config.dartpackages/ndk/pubspec.yamlpackages/ndk/test/data_layer/blob_cache/idb_blob_cache_manager_test.dartpackages/ndk/test/data_layer/blob_cache/noop_blob_cache_manager_test.dartpackages/ndk/test/entities/blob_nip94_test.dartpackages/ndk/test/usecases/files/blossom_blob_cache_test.dartpackages/ndk/test/usecases/files/blossom_test.dart
Comment on lines
+19
to
+33
| /// Persist a blob keyed by its SHA-256. | ||
| /// | ||
| /// If [sha256] is omitted it is computed from [data]. Pass it when | ||
| /// already known (e.g. right after a Blossom GET) to avoid hashing | ||
| /// twice. The caller is trusted not to lie about a provided hash. | ||
| /// | ||
| /// Re-saving an existing blob updates the descriptor metadata and | ||
| /// keeps the existing bytes. | ||
| Future<BlobDescriptor> saveBlob({ | ||
| required Uint8List data, | ||
| String? sha256, | ||
| String? mimeType, | ||
| String? sourceUrl, | ||
| BlobNip94? nip94, | ||
| }); |
There was a problem hiding this comment.
Consider verifying provided SHA-256 hashes.
Line 23 states "The caller is trusted not to lie about a provided hash." This trust assumption could enable cache poisoning if an untrusted caller provides an incorrect SHA-256 for blob data. An attacker could:
- Store blob A under the hash of blob B
- Cause cache hits to return incorrect content
- Potentially bypass content validation in downstream code
Recommendation: Consider one of:
- Always compute and verify the hash, ignoring the provided value (safest but less performant)
- Add a verification mode/flag for security-sensitive contexts
- Document the security implications and require callers to only pass pre-verified hashes from trusted sources (e.g., immediately after downloading from a Blossom server)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ndk/lib/domain_layer/repositories/blob_cache_manager.dart` around
lines 19 - 33, The saveBlob contract currently trusts the caller-provided sha256
which can enable cache poisoning; update the saveBlob implementation to compute
the SHA-256 of the provided data and compare it to the sha256 parameter (when
given), failing (throwing/returning an error) on mismatch, or alternatively add
an explicit verification flag (e.g., verifySha256) defaulting to true so callers
can opt out for performance; ensure the code path that updates/returns
BlobDescriptor enforces this verification and documents the behavior for callers
of saveBlob and consumers of BlobDescriptor.
Comment on lines
+342
to
+347
| if (cacheWrite) { | ||
| await _blobCache.saveBlob( | ||
| data: response.data, | ||
| sha256: sha256, | ||
| mimeType: response.mimeType, | ||
| ); |
There was a problem hiding this comment.
Make cache failures non-fatal in network success paths.
Line 342 and Line 596 currently let cache exceptions abort primary operations. A cache backend error can make getBlob fail after a successful download, and can block deleteBlob from even reaching the server.
Proposed fix
@@
- if (cacheWrite) {
- await _blobCache.saveBlob(
- data: response.data,
- sha256: sha256,
- mimeType: response.mimeType,
- );
- }
+ if (cacheWrite) {
+ try {
+ await _blobCache.saveBlob(
+ data: response.data,
+ sha256: sha256,
+ mimeType: response.mimeType,
+ );
+ } catch (_) {
+ // best-effort cache write; do not fail successful network read
+ }
+ }
@@
- await _blobCache.removeBlob(sha256);
+ try {
+ await _blobCache.removeBlob(sha256);
+ } catch (_) {
+ // best-effort cache invalidation; continue with remote delete
+ }Also applies to: 596-597
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ndk/lib/domain_layer/usecases/files/blossom.dart` around lines 342 -
347, The cache backend exceptions (e.g., from _blobCache.saveBlob) are currently
allowed to abort the primary network flows in getBlob and deleteBlob; change
both code paths so cache failures are non-fatal: wrap calls to
_blobCache.saveBlob (in getBlob) and any _blobCache.delete* calls (in
deleteBlob) in try/catch blocks, log a warning/error on failure, and do not
rethrow so the successful network download or server delete still completes;
ensure the server/network operation remains the primary awaited action and cache
cleanup is best-effort only.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #606 +/- ##
==========================================
+ Coverage 73.87% 74.21% +0.33%
==========================================
Files 205 207 +2
Lines 10901 11040 +139
==========================================
+ Hits 8053 8193 +140
+ Misses 2848 2847 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
frnandu
requested changes
May 21, 2026
Collaborator
frnandu
left a comment
frnandu
left a comment
There was a problem hiding this comment.
Confirm that there are some kind of memory limits in the idb_shim lib , otherwise the cache will grow without bounds. Provide documentation on how to configure such limits
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
2 participants
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.
Like Events Cache Manager but for blobs
Summary by CodeRabbit
Release Notes
New Features
cacheWriteparameter to blob operations for granular caching control.Documentation
Tests