Fix to store added files in remotes and fetch from remotes if missing locally #1375
Conversation
WalkthroughAdds structured error logging around CRDT file decoding, implements remote-loading fallback and local caching when CARs are missing, and reinstates/extends remote propagation for WAL and file upload processing with loader existence guards. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Loader
participant Local as Local CarStore/FileStore
participant Remote as Remote Stores
participant Store as BlockStore
participant CRDT as CRDT decoder
Client->>Loader: loadCar(cid)
Loader->>Local: load(cid)
alt Local found
Local-->>Loader: car bytes
Loader->>Client: loadedCar
else Local not found / error
rect rgb(220,240,230)
Note over Loader,Remote: Remote fallback
Loader->>Remote: iterate remotes -> load(cid)
alt Remote succeeds
Remote-->>Loader: car bytes
Loader->>Local: save(car) -- cache locally
Local-->>Loader: saved
Loader->>Client: loadedCar
else Remote fails (each)
Remote-->>Loader: error
Note over Loader: log failure, continue
end
end
end
Client->>Store: processWAL/op(file)
Store->>Store: ensure this.loader exists (guard)
Store->>Local: handle local save
Store->>Remote: forRemotes: save(car/file, publicFlag)
Remote-->>Store: ack / error (logged)
sequenceDiagram
participant Reader
participant CAR as CAR bytes
participant CRDT as Decoder
Reader->>CRDT: decodeFile(block)
alt decode success
CRDT-->>Reader: file bytes
else decode throws
CRDT->>CRDT: catch error
CRDT->>CRDT: log {car, cid, errorMessage, errorStack}
CRDT-->>Reader: throw structured error {cid, car, fileName, error, stacks}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 1
🧹 Nitpick comments (3)
core/base/crdt-helpers.ts (1)
252-299: Improved file decode error logging looks good; consider including filename and avoiding redundant checksThe new try/catch around
blocks.getFileand the richerdecodeFileerror path both add useful context (car, cid, message, stack, original error) and surface failures as proper logged errors, which aligns well with the remote-loading/WAL flows.Two small optional tweaks:
- You already guard on
if (fileMeta.car)before assigningfileMeta.file, so usingthrowFalsy(fileMeta.car)multiple times is a bit redundant; capturingconst car = throwFalsy(fileMeta.car)once and reusing it would both clarify the invariant and avoid re-throwing ifcarwere somehow mutated later.- In the decode error log,
fileNameis populated fromfileMeta.type, which is typically a MIME type rather than the logical filename. If available, logging thefilenamefrom the outer loop (or adding a dedicatedfileNamefield inDocFileMeta) would make these logs easier to correlate to user-facing files.core/blockstore/store.ts (1)
491-623: WAL remote propagation logic is solid; verify loader-null guard and file-only processing behaviorThe refactor in
_doProcessachieves the intended behavior:
noLoaderOpsandoperationsnow push cars to all remotes viathis.loader.attachedStores.forRemotes(...), which replaces the older singlexremoteCarStorepath.fileOperationscorrectly load the local file block once and then save it to each remote file store with thepublicflag, and prune entries fromwalState.fileOperations.- After successfully handling
operations, pushinglastOpto each remote meta store viaforRemoteskeeps remote meta in sync.- Using
retryableUploadwithpRetryand a smallconcurrencyLimitis a good balance between robustness and load.A couple of points worth double-checking:
this.loaderis required in the constructor, so the earlyif (!this.loader) return;and the repeatedif (!this.loader) return;checks inside eachretryableUploadshould never trigger in normal usage. If they do ever trigger (e.g., due to future refactors or type casts),_doProcesswill exit without touchingwalState, whileprocess()continues to re-schedule itself as long as there are pending operations, which could cause a silent retry loop. If you expectloaderto sometimes be absent, consider logging and skipping the re-schedule in that case, or clearingwalStateexplicitly.enqueueFileonly appends tothis.walState.fileOperationsand does not callprocess(). That means file uploads will be flushed to remotes only when some other code path also enqueues a non-noLoadercommit (which does triggerprocess()). Please confirm that every caller that usesenqueueFileis paired with a correspondingenqueue/commit, or consider invokingvoid this.process()fromenqueueFileas well if immediate propagation of standalone file uploads is important.core/blockstore/loader.ts (1)
826-915: Remote fallback inmakeDecoderAndCarReadermatches intent; avoid double local save and duplicate castsThe new flow nicely covers the “missing locally, fetch from remotes, then cache locally” story:
- Local
activeStore.load(carCid)first, with non-isNotFoundErrorexceptions surfaced as logged errors.- On NotFound, you walk
store.remotes(), tryremote.load(carCid), and when successful, use that store’s codec viaawait activeStore.keyedCrypto()and mark the item as originating from the remote store.- Decoding through
asyncBlockDecodeand feedingCarReader.fromBytesfrom the decryptedbytes.value.datafixes the earlier mismatch wherefromBytesexpected raw Uint8Array car bytes.Two refinements to consider:
Avoid saving the same remote car to local twice
In the remote-success path you currently:
const remoteCar = await remote.load(carCid); if (remoteCar) { this.logger.Debug().Ref("cid", remoteCar.cid).Msg("saving remote car locally"); await store.local().save(remoteCar); loadedCar = remoteCar; activeStore = remote; break; } ... if (activeStore !== store.local()) { await store.local().save(loadedCar); }When a remote load succeeds,
store.local().save(...)is invoked both inside the loop and again in theactiveStore !== store.local()block. It’s harmless but causes redundant IO. You can drop the first save and keep a single “save once” after the try/catch, moving the debug log there, for example:- const remoteCar = await remote.load(carCid); - if (remoteCar) { - this.logger.Debug().Ref("cid", remoteCar.cid).Msg("saving remote car locally"); - await store.local().save(remoteCar); - loadedCar = remoteCar; - activeStore = remote; - break; - } + const remoteCar = await remote.load(carCid); + if (remoteCar) { + loadedCar = remoteCar; + activeStore = remote; + break; + } ... - if (activeStore !== store.local()) { - // if coming from remote store, save it locally not in cache - await store.local().save(loadedCar); - } + if (loadedCar && activeStore !== store.local()) { + this.logger.Debug().Ref("cid", loadedCar.cid).Msg("saving remote car locally"); + await store.local().save(loadedCar); + }DRY up and type the decoded bytes access
You access
(bytes.value as { data: Uint8Array }).datatwice (forCarReader.fromBytesand the returnedbytesfield). To make this clearer and safer, consider extracting once with a typed helper/local:const decoded = bytes.value as { data: Uint8Array }; const carBytes = decoded.data; const rawReader = await CarReader.fromBytes(carBytes); ... return { cid: carCid, bytes: carBytes, item: { ... }, };This avoids repeating the cast and makes the expected shape of
bytes.valueexplicit.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/base/crdt-helpers.ts(1 hunks)core/blockstore/loader.ts(5 hunks)core/blockstore/store.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-13T14:08:24.069Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 801
File: src/blockstore/loader.ts:401-403
Timestamp: 2025-05-13T14:08:24.069Z
Learning: In the `handleMetaStream` function in `src/blockstore/loader.ts`, recursive calls intentionally omit the `opts` parameter to avoid calling the `.first` callback multiple times, even though this also means the error handler won't be called for subsequent read errors.
Applied to files:
core/base/crdt-helpers.ts
🧬 Code graph analysis (3)
core/base/crdt-helpers.ts (1)
core/types/base/types.ts (1)
throwFalsy(71-76)
core/blockstore/store.ts (1)
core/blockstore/loader.ts (1)
carLogIncludesGroup(55-68)
core/blockstore/loader.ts (1)
core/types/blockstore/types.ts (2)
CarStore(462-467)FileStore(469-474)
🪛 ESLint
core/blockstore/store.ts
[error] 32-32: 'throwFalsy' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/blockstore/store.ts (1)
470-474: Critical: Missing walState save in enqueueFile may cause data loss.Unlike the
enqueuemethod (line 464),enqueueFiledoes not save the walState after modifying it. This creates a window where fileOperations could be lost if the process crashes before_doProcessexecutes and saves the state (line 621).For consistency and durability,
enqueueFileshould persist the walState immediately after modification, matching the pattern inenqueue.Apply this diff to ensure durability:
async enqueueFile(fileCid: AnyLink, publicFile = false) { await this.ready(); this.walState.fileOperations.push({ cid: fileCid, public: publicFile }); - // await this.save(this.walState) + await this.save(this.walState); }
🧹 Nitpick comments (1)
core/blockstore/store.ts (1)
522-524: Consider adding debug logging to loader existence guards.The loader existence checks are good defensive programming and prevent potential null/undefined errors. However, the silent early returns could make debugging harder if the loader is unexpectedly missing during processing.
Consider adding debug logging to help diagnose issues:
if (!this.loader) { + this.logger.Debug().Msg("Loader not available, skipping processing"); return; }Also applies to: 551-553, 579-581, 603-605
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/blockstore/store.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-29T11:19:21.299Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 801
File: src/use-fireproof/index.ts:19-22
Timestamp: 2025-05-29T11:19:21.299Z
Learning: In src/use-fireproof/index.ts, the defaultChanged function intentionally throws an error as part of a lazy initialization pattern. This error should only occur if the lazy logic fails to properly replace the default handler with actual event handlers during setup. The error serves as a fail-fast mechanism to catch initialization problems early.
Applied to files:
core/blockstore/store.ts
🧬 Code graph analysis (1)
core/blockstore/store.ts (1)
core/blockstore/loader.ts (1)
carLogIncludesGroup(55-68)
🔇 Additional comments (4)
core/blockstore/store.ts (4)
532-536: LGTM! Remote propagation for noLoaderOps aligns with PR objectives.The addition of remote propagation logic ensures that locally available cars are pushed to all registered remote stores. This implementation correctly handles the case where cars exist locally and need to be synchronized.
560-565: LGTM! Remote propagation for operations implemented correctly.The remote propagation logic for regular operations mirrors the noLoaderOps implementation and correctly ensures cars are synchronized to all remote stores.
577-577: LGTM! Public flag handling for file operations is correct.The implementation correctly destructures the
publicflag from file operations and passes it through to the remote save operations, enabling proper access control for files stored in remotes.Also applies to: 586-589
606-609: LGTM! Remote metadata save completes the synchronization flow.The remote metadata save is correctly positioned after all car and file uploads complete, ensuring remote stores receive consistent metadata updates. This completes the remote synchronization implementation as described in the PR objectives.
|
Thanks for submitting this --- there are some conceptual things which you missed --- It would be cool if I could explain them to you --- pls contact me on Discord @fastandfearless or fast&fear<. |
2 participants
As discussed on Discord, the sync of files got broken in recent codebase changes. This change pushes locally added files to the registered remotes and uses the remotes as fallback if an attachment does not yet exist locally.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.