Skip to content

[CHIA-4324] NFT media pipeline 5: download timeout recovery, bounded confirmation previews, buffer cleanup - #3033

Open
jlobue10 wants to merge 10 commits into
Chia-Network:mainfrom
jlobue10:nft-5-download-resilience
Open

[CHIA-4324] NFT media pipeline 5: download timeout recovery, bounded confirmation previews, buffer cleanup#3033
jlobue10 wants to merge 10 commits into
Chia-Network:mainfrom
jlobue10:nft-5-download-resilience

Conversation

@jlobue10

@jlobue10 jlobue10 commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Part 5 of the NFT media pipeline series (#3011#3010#3029). This PR is meant to be applied after #3029: it is stacked on that branch, so until #3029 merges its diff includes #3029's 7 commits — review only the 3 commits listed below. Each is independent of the others; together they harden the download path that the earlier PRs put in place.

What this PR changes and why

1. Retry timed-out NFT downloads once per session (46ca7ecb9)

Settling download timeouts as permanent cache errors (#3010) fixed the retry-a-stalled-host-on-every-access loop, but overshot: the timeout message is not in the transient-error list, so one slow first byte from a cold gateway — or a briefly offline machine — wrote an error to the -info sidecar that survived restarts. The NFT showed "Preview is not available" forever, until the user cleared the entire NFT cache.

CacheManager now keeps an in-memory timedOutUrls set: a persisted timeout is retried once per app session. Within the session the URL settles after its first timeout (a stalled host still cannot hold a download slot on every gallery visit), but a restart gets a fresh attempt. downloadFile exports isDownloadTimeoutError, which matches both timeout messages it produces (inactivity and overall deadline) by the same prefixes earlier sessions persisted — so existing poisoned caches recover on their own.

2. Bound WalletConnect confirmation-preview resolution with one overall deadline (f68778eb5)

resolveNftPreviewUrl walks three ordered URI lists per NFT (image data URIs, metadata URIs, extensionless data URIs) with a 10 s timeout per fetch and no overall limit. URI lists are on-chain data that nft_add_uri can extend, and the WalletConnect confirmation dialog only opens after parsing settles — so a take_offer referencing NFTs with many dead or slow hosts kept the security dialog off screen for 10 s × URIs × lists, minutes in the worst case, while the request could expire.

All fallbacks for one NFT now share a 20 s budget: nftGetMetadata / nftGetImageDataUrl take a timeoutBudget, each fetch gets the remaining time (capped at its own per-fetch timeout), and the walk stops once the budget is spent. Previews degrade to the placeholder in that case — the dialog itself is never held up by more than the budget. NFTs resolve concurrently, so this bounds the whole parse regardless of how many NFTs an offer references.

3. Drop redundant buffer copies and a dead hook export (72a9b8d58)

The verified-preview path copied every downloaded byte twice for no gain: fetchBuffer re-allocated each response chunk element-by-element via Uint8Array.from before collecting it (Electron's net module delivers fresh Buffers, so retaining them is safe and Buffer.concat already performs the single final copy), and the metadata checksum round-tripped the whole buffer through a latin1 string before hashing — an up-to-10 MB string allocation to produce the digest that hashing the Buffer directly yields.

Also removes the unused useNFTVideoLoop default export: both consumers import the named hooks and derive the effective global-or-per-video state themselves, so the wrapper only added surface that could drift from the real logic.

No user-facing strings or settings are added; no cache migration is needed (cache keys and sidecar formats are unchanged).

Testing

  • New CacheManager test: a timeout persisted by one session (Request timed out after …ms of inactivity in the -info sidecar) is retried, and succeeds, in a fresh CacheManager instance against the same cache directory.
  • New parseCommandDisplay test: once a slow metadata host consumes the whole budget, the remaining metadata fallbacks are never tried and the walk resolves to the placeholder; the existing resolution tests now also assert that a numeric budget is passed through to every nftGetMetadata / nftGetImageDataUrl call.
  • Full gui jest suite green (355 tests); eslint/prettier clean on all touched files.

🤖 Generated with Claude Code

https://claude.ai/code/session_01YKEGodEgVdEuvUM8dza52q


Note

Medium Risk
Touches NFT fetch, cache retry, and WalletConnect confirmation-preview timing. Gateway use is opt-in and hash-verified, but it changes which URLs leave the machine and how confirmation dialogs wait on network.

Overview
Adds an opt-in NFT setting to fetch ipfs:// media through https://ipfs.io. Off by default: Electron cannot request the ipfs: scheme, so those URIs are not fetched unless the user enables it. Content is still hash-checked against the on-chain hash; the original URI remains the cache key.

Network call sites (downloadFile, fetchBuffer, fetchJSON, Chromium downloads) go through toFetchableUrl. Gateway-disabled refusals are not persisted as cache errors so turning the option on retries cleanly. Failed in-memory metadata and on-screen hash verification also retry when the toggle flips.

Also hardens the download/preview path: persisted timeouts retry once per session; WalletConnect confirmation previews share a 20s fallback budget so slow URI lists cannot stall the security dialog; fetchBuffer/checksum drop extra copies; unused useNFTVideoLoop default export is removed.

Reviewed by Cursor Bugbot for commit 72a9b8d. Bugbot is set up for automated code reviews on this repo. Configure here.

jlobue10 and others added 10 commits August 21, 2026 09:19
Some NFTs are minted with bare ipfs://<CID>/<path> data, metadata, or
license URIs instead of an HTTPS gateway URL. Every such URI failed
validation in the GUI cache layer with "Invalid URL: ipfs://...":
validator's isURL applies an FQDN check to the host, and a CID has no
top-level domain, so listing 'ipfs' as an allowed protocol never
actually accepted anything. Even when a caller ignored the validation
error, Electron's net stack cannot request the ipfs scheme, so the
media could never be fetched, verified, or cached.

Translate ipfs:// URIs to their HTTPS gateway form
(https://ipfs.io/ipfs/<CID>/<path>) in one shared helper and apply it

- in the electron isValidURL, which now validates the gateway form (the
  URL that is actually requested),
- at the outgoing request sites (downloadFile, fetchBuffer, fetchJSON)
  right where the URL reaches net.request,
- at the single-NFT download handler, whose Chromium downloadURL cannot
  fetch the ipfs scheme either,
- in the oversized-image direct-URL fallback of the dapp dialog, whose
  CSP only allows https: and data: images,
- in the NFTHashStatus badge so ipfs URIs are no longer flagged as
  invalid in the renderer.

The original on-chain URI remains the cache key everywhere, so existing
cache entries, cache-info sidecars, and renderer lookups are unaffected.
The redundant ipfs://ipfs/<CID> form produced by some minting tools is
tolerated, and CID case is preserved (CIDv0 is case-sensitive base58).
The gateway does not need to be trusted for integrity: everything the
cache serves is verified against the NFT's on-chain hash.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vai5yNzPZwUSgid2nhQvys
Review feedback on the gateway feature: the gateway URL is technically
not the URI recorded on chain, so translating ipfs:// URIs to
https://ipfs.io/ipfs/... should be something the user opts into rather
than automatic behavior.

- New 'Fetch IPFS content through a gateway' switch in Settings > NFT,
  off by default. While off, ipfs:// URIs behave as before the gateway
  feature: they fail URL validation and are never fetched, and the
  NFTHashStatus badge flags them again.
- The preference is stored as nftIpfsGateway via the existing
  prefs.yaml round-trip; the main process reads the persisted value
  through electron/utils/ipfsGateway.ts (fail-closed when the store is
  unreadable) at every site that translated URLs: isValidURL,
  downloadFile, fetchBuffer, fetchJSON, the single-NFT download
  handler, and the dapp dialog's oversized-image fallback. The ipfs
  scheme check runs before the preference read so non-ipfs requests
  never touch the store.
- The oversized-image fallback returns no preview (instead of a
  CSP-blocked raw ipfs URI) while the option is off.
- Nothing is persisted for a rejected ipfs URL (the outer isValidURL
  guard throws before the cache-info error sidecar is written), so
  enabling the option retries previously failing NFTs cleanly.
- util/ipfs.ts stays a pure translation helper shared with the
  renderer; the preference gate lives only in the electron layer and
  the useIpfsGateway hook.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LdfCqRSBWwMpCDh1SdE24e
isValidURL treated ipfs:// URIs as invalid whenever the gateway option
was off, and CacheManager consults that check before every cache path
lookup. Content that was downloaded and hash-verified while the option
was on therefore became unservable the moment it was switched off —
the cached bytes could not be served, checksummed, or evicted even
though serving a local file involves no gateway request (Bugbot,
PR Chia-Network#3029).

- isValidURL is now a structural check only: ipfs URIs are validated
  via their gateway form regardless of the preference.
- Fetching is gated where it happens instead: downloadFile,
  fetchBuffer, and fetchJSON resolve their request URL through a new
  toFetchableUrl, which throws IpfsGatewayDisabledError for ipfs URIs
  while the option is off.
- CacheManager rethrows that error instead of persisting it as a cache
  ERROR entry, so flipping the option on retries cleanly - a persisted
  'disabled' error would have poisoned the entry (only transient errors
  are ever retried).
- The single-download IPC handler drops ipfs URLs while the option is
  off instead of handing Chromium a URL it silently fails on.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017NXWAjaHb9SFafLeguTHd8
Enabling 'Fetch IPFS content through a gateway' had no effect on NFTs
already on screen: useNFTVerifyHash never re-ran (nothing depended on
the preference), and a failed ipfs metadata fetch stayed cached in the
NFT provider, so those NFTs kept looking broken until a full app
reload (Bugbot, PR Chia-Network#3029).

- Both verification effects in useNFTVerifyHash now list the
  preference as a dependency, so flipping it re-checks data and
  preview URIs immediately.
- useMetadataData retries cached metadata failures when the preference
  flips - only failures: successfully fetched metadata is hash-verified
  content and unaffected by how it was fetched. The retry goes through
  invalidate, whose refetch notifies mounted subscribers.

The cache layer needs no matching change: gateway-disabled fetch
refusals are never persisted, so the re-run's fresh requests go
through cleanly.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017NXWAjaHb9SFafLeguTHd8
The gateway-aware validity check in NFTHashStatus sat behind
'originalUri' in nftPreview, but NFTPreviewState has no originalUri
field, so the guard always returned early: isValidURI stayed true, the
'URL is not valid' badge never showed for unfetchable ipfs URIs, and
the ipfsToGatewayUrl path was dead code (Bugbot, PR Chia-Network#3029).

The check now validates nftPreview.uri directly. Message precedence is
unchanged: a file that already verified from the cache still reports
'Hash matches' - the URL branch is only reached for unverified states,
which is exactly when an unfetchable URI is the thing worth reporting.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017NXWAjaHb9SFafLeguTHd8
The gateway-flip retry effect only invalidated cache entries that had
already failed. An entry whose fetch was still in flight was skipped —
so a request started while the option was off could reject after the
toggle had run, caching a failure that nothing would ever retry until
remount (Bugbot, PR Chia-Network#3029).

In-flight entries now get a rejection handler: if the pending fetch
fails, it is invalidated and refetched under the new preference, while
a result that arrives successfully is kept instead of being thrown away
and refetched. Repeated toggles can stack handlers on one promise, but
each retry goes through invalidate, so the worst case is a redundant
refetch, not an inconsistent cache.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017NXWAjaHb9SFafLeguTHd8
The gateway-flip retry effect had two churn paths (Bugbot, PR Chia-Network#3029):
retrying an errored entry re-inserts its key with a fresh in-flight
promise synchronously, and Map.forEach revisits keys re-added during
the pass - so the effect attached a rejection retry to the very fetch
it had just started, double-fetching a failure. And rapid toggles could
stack rejection handlers on one promise; when it rejected, each handler
invalidated in turn, the later ones discarding the refetch the first
had started - even a successful one.

The effect now iterates a snapshot of the map, and a rejection handler
retries only the failure it saw: the fetch's own catch stores its
rejection as the entry's error, so an entry that has moved on - already
retried by a stacked handler, or settled successfully - is left alone.
Each failure is retried exactly once per flip and successful results
are never dropped.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017NXWAjaHb9SFafLeguTHd8
Settling download timeouts as permanent cache errors fixed the
retry-a-stalled-host-on-every-access loop, but overshot: the timeout
message is not in the transient-error list, so one slow first byte from
a cold gateway — or a briefly offline machine — wrote an error that
survived restarts. The NFT showed "Preview is not available" forever,
until the user cleared the entire NFT cache.

A persisted timeout is now retried once per app session: within the
session the URL settles after its first timeout (a stalled host still
cannot hold a download slot on every gallery visit), but a restart gets
a fresh attempt, so a one-off network problem no longer bricks the
preview. Timeout detection matches the messages persisted by earlier
sessions' -info files, so existing poisoned caches also recover.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FUAhsJYiWuD36Hq4h49gLg
resolveNftPreviewUrl walks three ordered URI lists per NFT (image data
URIs, metadata URIs, extensionless data URIs) with a 10-second timeout
per fetch and no overall limit. NFT URI lists are on-chain data that
nft_add_uri can extend, and the WalletConnect confirmation dialog only
opens after parsing settles — so a take_offer referencing NFTs with many
dead or slow hosts kept the security dialog off screen for
10s x URIs x lists, minutes in the worst case, while the request could
expire.

All fallbacks for one NFT now share a 20-second budget: each fetch gets
the remaining time (capped at its own per-fetch timeout) and the walk
stops when the budget is spent. Previews degrade to the placeholder
in that case — the dialog itself is never held up by more than the
budget. NFTs resolve concurrently, so the budget bounds the whole
parse regardless of how many NFTs an offer references.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FUAhsJYiWuD36Hq4h49gLg
The verified-preview path copied every downloaded byte twice for no
gain: fetchBuffer re-allocated each response chunk element-by-element
via Uint8Array.from before collecting it (Electron's net module
delivers fresh Buffers, so retaining them is safe and Buffer.concat
already performs the single final copy), and the metadata checksum
round-tripped the whole buffer through a latin1 string before hashing,
which allocates an up-to-10MB string to produce the digest hashing the
Buffer directly yields.

Also removes the unused useNFTVideoLoop default export: both consumers
import the named hooks and derive the effective global-or-per-video
state themselves, so the wrapper only added surface that could drift
from the real logic.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FUAhsJYiWuD36Hq4h49gLg
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants