[CHIA-4324] NFT media pipeline 1/3: cache & download infrastructure - #3008
[CHIA-4324] NFT media pipeline 1/3: cache & download infrastructure#3008jlobue10 wants to merge 10 commits into
Conversation
|
CI didn't seem to run on this - so closing and reopening to get fresh CI run |
|
I'm not sure it matters, but this three part PR is meant to go all as one larger PR. It was split up at the request of a few people. I've been using this modified version of the GUI for a little while and haven't seen and deal breaking bugged behavior for the issues these are addressing. This 3 part series may also need a rebase, with other merged fixes incorporated, since these patches were made. Easy enough to do, if necessary. |
|
close and reopen for new CI with new packages |
|
@jlobue10 please address the latest Bugbot comments |
|
@cursor review |
|
@danieljperry Done — all of the latest Bugbot findings across the stack are addressed: This PR (#3008):
Downstack: #3011's multi-select remount is fixed on its branch (the loop-toggle half is inherent to the Each branch tip re-verified: full gui Jest suite green (296/296 here, 331/331 at the top of the stack), eslint/prettier clean, and no new TypeScript errors (the gui package's pre-existing baseline is unchanged). No dependencies were touched, so the |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 11c2572. Configure here.
Thanks for this. Getting closer. PRs #2 and #3 have new bugbot review comments. |
|
@jlobue10 we'll move to human reviews next. Thanks! |
Video and audio NFTs frequently render as a blank tile while images work. The media pipeline had several defects that disproportionately hit video: - The cache: scheme was never registered with registerSchemesAsPrivileged, so media elements expected a buffered response and stalled on the streamed body served by protocol.handle. Register it with stream: true (plus standard/secure/supportFetchAPI) before app ready. - The cache protocol handler ignored Range requests, returning 200 with the full file. Seeking was broken for all media and MP4 files without faststart (moov atom at the end) could fail to start playing at all. Serve 206 partial content from the cached file with accept-ranges. - The 30s download timeout was a total-transfer budget, so any video that could not be fully downloaded in 30s never displayed. Make it an inactivity timeout that resets on each received chunk. - Aborts caused by the 100MB size cap surfaced as generic "Request aborted"/"Response aborted" errors, which are treated as transient, so oversized files were re-downloaded on every gallery visit. Report a distinct "Maximum file size exceeded" error, persist it, and only retry when the caller lifts the size limit. - maxSize <= 0 now disables the size limit instead of aborting every download on the first chunk, fixing the ignoreSizeLimit path (useNFTVerifyHash passes maxSize: -1) and NFTPreview now forwards the same override to getURI so verification and display stay consistent. - Drive-by: download progress used Math.max instead of Math.min and was pinned at 100%. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The settings UI stored the cache size limit under the cacheLimitSize preference key, but on startup the main process reads maxCacheSize - a key nothing ever wrote - so a user-configured cache size silently reverted to the 1GB default on every launch. Changing the cache folder was never persisted at all. Persist both values from CacheProvider when the corresponding change events arrive from the main process. The renderer is the only safe place to write them: prefs.yaml is rewritten from the renderer's full preferences snapshot on every save, so a value written by the main process would be clobbered by the next renderer preference change. On startup the legacy cacheLimitSize key is still honored so existing users keep their configured limit, and non-positive stored values are ignored instead of crashing the CacheManager constructor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The inactivity-based download timeout left transfers with no upper bound: a host trickling one byte per interval could hold a download concurrency slot indefinitely. Downloads now also have an absolute 30-minute deadline, reported as the same transient abort error so legitimate retries still work. (Split from "Harden NFT preview interactions and download bounds" — the gallery multi-select interaction fix is in the video/UX PR.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T88GQEqcbujpAKLZy3sKoq
Cache-eviction portion of a hardening pass over the NFT media branch: - Eviction after a completed download now evicts down to the configured total-size target while preserving the file that was just downloaded, instead of removing an ad-hoc amount that could include it. - Eviction accounting includes each entry's info sidecar so it matches getCacheSize(). - setCacheInfo is awaited, so the cache info returned to callers is the saved state rather than a pending promise. - sanitizeNumber accepts 0 (meaning unlimited) and rejects negative or non-finite sizes; the stored preference is accepted when >= 0. (Split from "fix: harden NFT previews and cache eviction" — the preview verification portion is in the preview-hardening PR.)
NFT invalidations (driven by nft_coin_added events while the wallet syncs) delete cache files concurrently with downloads. The cache-size scan that runs after every download stats each file with no tolerance for concurrent deletion, so one vanished file rejected the whole scan — and the download's catch block then overwrote its just-saved CACHED state with the unrelated ENOENT error. The gallery re-downloaded those "failed" files, triggering more invalidations and gateway rate limiting (HTTP 429/504) until the GUI became unusable. Vanished files are now skipped by both the size and eviction scans, post-download housekeeping failures are logged instead of replacing the completed download's state, and per-file sizeChanged bursts coalesce into one trailing renderer notification instead of a full directory scan each. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T88GQEqcbujpAKLZy3sKoq
Download timeouts (inactivity and the absolute deadline) aborted the request without recording a reason, so they persisted as the generic "Request aborted" — which the cache treats as transient and retries on every access. Dead hosts therefore stalled every gallery pass for the full timeout, forever. Timeouts now settle as failed downloads with their own message; genuine caller-driven aborts remain retryable. (Split from "Unblock NFT verification from slow metadata and settle timed-out downloads" — the verification-hook change is in the preview-hardening PR.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T88GQEqcbujpAKLZy3sKoq
The download queue was strictly FIFO, so anything the user opened while a large backlog was queued — an offer preview, a detail page — waited behind every earlier request. With a cold cache the gallery enqueues thousands of downloads and dead hosts hold slots for the full timeout, leaving on-demand previews stuck for many minutes. The download limiter now runs the most recently requested task first (per-URL deduplication makes ordering safe), so downloads for what the user is currently viewing start on the next free slot. RPC paging users of the limiter keep FIFO ordering. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T88GQEqcbujpAKLZy3sKoq
A download aborted by invalidation or a cache directory change while it was still waiting in the concurrency limiter previously started anyway: downloadFile only listened for future abort events and never checked signal.aborted. The raced transfer held a download slot and, once the inactivity or deadline timeout hit, could settle the URL with a permanently persisted timeout error. Reject up front with the same retryable 'Request aborted' error the mid-flight abort path reports. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Q2tBCuePZJeeCkDDxu1m3
Older builds stored the cache limit under the legacy cacheLimitSize key and rejected zero at startup, keeping the 1GB default. Accepting zero from that key would silently turn those prefs files into an unlimited cache after upgrade. Only the current maxCacheSize key may carry zero (unlimited); a legacy zero falls through to the default. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Q2tBCuePZJeeCkDDxu1m3
The sizeChanged coalescer cleared its pending flag when the timer fired, so a burst arriving while a scan was still running scheduled another scan that overlapped the in-flight one on large caches. Track the running scan, mark it stale when events arrive mid-scan, and run a single follow-up scan after it settles so scans never overlap and the trailing notification still delivers a fresh size. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Q2tBCuePZJeeCkDDxu1m3
11c2572 to
89c9716
Compare
|
@jlobue10 looks like we had a bit of a miscommunication at merge time - PR 2 got merged so I'm not sure what state this is in now |
|
Yea, just an order snafu - so right now 2 is merged. but 1, 3 and 4 are not - however, you did say that 2 included the 1 commits anyway, so ? not sure about your work you just did a few mins ago - this was ready to go so I would have suggsted to avoid further iterations. |
|
Sorted out — no harm done, and nothing needs to be re-reviewed: Because these PRs were stacked, #3011's branch contained all of this PR's commits, and the squash merge (2a916d4) therefore carried the entire content of this PR into main along with 2/3's own changes. I verified the files this PR owns (CacheManager, downloadFile, the cache prefs/eviction/queue work) are byte-identical between main and this branch tip — every commit here would rebase to empty. So the merge order didn't lose anything; 1/3 and 2/3 simply landed together. Closing this PR as merged-via-#3011. #3010 (3/3) and #3029 (4/4) have been rebased onto the new main so they now carry only their own commits and are conflict-free — they're ready for review/CI whenever you are. |
Part 1 of 3 of the split of #2993, as discussed with @danieljperry. These three PRs are meant to be applied together, in order (1/3 → 2/3 → 3/3): each later PR is stacked on the previous one's branch, so its diff will include the earlier PRs' commits until they merge. Reviewing part 1 first keeps each review small. The combined final tree is byte-identical to #2993 rebased onto current main (verified with
git diff— empty).What this PR changes and why
This is the electron-side media pipeline: the
cache:protocol, the download layer, and cache lifecycle management. It fixes the root causes of video/audio NFTs rendering as blank tiles, plus several cache defects found while testing against a real 231-NFT wallet.Fix NFT video/audio playback in the GUI
The headline playback bug was an infrastructure bug:
cache:scheme was never registered withregisterSchemesAsPrivileged, so media elements stalled on the streamed body served byprotocol.handle. It is now registered withstream: true(plus standard/secure/supportFetchAPI) before app ready.Rangerequests, so seeking was broken for all media and MP4s without faststart could fail to start at all. It now serves 206 partial content withaccept-ranges.maxSize <= 0disables the limit (fixing theignoreSizeLimitpath).Persist NFT cache size and folder across GUI restarts
Cache settings changes only lived in CacheManager memory. They are persisted from the renderer, which is the sole prefs writer — a main-process write would be clobbered by the next renderer snapshot save (prefs.yaml is rewritten whole).
Bound NFT downloads with an absolute 30-minute deadline
The inactivity timeout alone let a host trickling one byte per interval hold a download concurrency slot indefinitely.
fix: harden cache eviction accounting and size handling
Eviction after a download now evicts down to the configured target while preserving the just-downloaded file, counts info sidecars like
getCacheSize()does, awaitssetCacheInfo, and treats a stored size of 0 as unlimited.Stop cache races from poisoning downloads and flooding the renderer
Found live on a wallet where NFT invalidations (driven by
nft_coin_addedduring wallet sync) race concurrent downloads: the post-download cache-size scan statted every file with no tolerance for concurrent deletion, and one vanished file rejected the scan — the download's catch block then overwrote the just-saved CACHED state with an unrelated ENOENT error. The gallery re-downloaded those "failed" files, snowballing into gateway rate-limiting. Scans now skip vanished files, housekeeping failures cannot replace a completed download's state, and per-filesizeChangedbursts coalesce into one trailing notification (each one previously triggered full directory scans in main and renderer).Settle timed-out downloads instead of retrying them forever
Timeouts aborted without recording a reason, persisting as the transient "Request aborted", so dead hosts (e.g. the shut-down nftstorage.link gateway) stalled every gallery pass for the full timeout, forever. Timeouts now settle as failed downloads; genuine caller-driven aborts remain retryable.
Serve the newest cache downloads first so visible content is not starved
The FIFO download queue made anything opened during a large backlog (an offer preview with a cold cache) wait behind thousands of earlier requests, with dead hosts holding slots for the full timeout. The limiter now supports LIFO ordering, enabled only for the download queue (safe:
fetchRemoteContentdeduplicates per URL); RPC-paging users keep FIFO.Testing
CacheManager.test.ts(new): eviction preservation, vanished-file tolerance during size/eviction scans (broken symlinks reproduce the race deterministically), housekeeping-failure isolation, timeout-vs-abort retry semantics, zero-means-unlimited.limit.test.ts(new): FIFO default and LIFO ordering.tsc --noEmiterror count ≤ main's baseline; eslint/prettier clean.🤖 Generated with Claude Code
Note
Medium Risk
Touches Electron custom-protocol serving, download timeouts, and cache eviction—core NFT media infrastructure. Bugs here can stall playback, leak download slots, or wipe completed cache entries.
Overview
Makes NFT media actually play from
cache://and stops cache races from poisoning completed downloads.Registers the
cachescheme as privileged withstream: truebefore app ready, and serves HTTP Range / 206 from the protocol handler so seeking and non-faststart MP4s work. Downloads now use an inactivity timeout plus a 30-minute hard deadline, skip already-aborted queued work, persist size-limit failures (retry only when the caller lifts the cap), and treat timeouts as permanent errors instead of retrying forever.Cache lifecycle is more robust: eviction preserves the just-downloaded file, counts sidecar metadata, ignores vanished files during scans, and cannot overwrite a successful download if housekeeping fails. Size-change notifications are coalesced and serialized. Downloads run LIFO so visible previews are not starved. Cache size/folder are persisted from the renderer (
maxCacheSize/cacheFolder, with legacycacheLimitSizemigration);0means unlimited.NFTPreviewpassesmaxSize: -1when ignoring the size limit.Reviewed by Cursor Bugbot for commit 89c9716. Bugbot is set up for automated code reviews on this repo. Configure here.