feat(find): shared HTTP deadlines everywhere + visible indexer progress - #101
Conversation
Three layers, one root cause. Node's happy-eyeballs gives each address-family connect attempt 250 ms — shorter than one round trip to api.raster.art from Asia (~250 ms RTT). On networks with broken IPv6 the IPv4 attempt dies mid-handshake, the IPv6 fallback has no route, and find fails or stalls with no output. - entry: raise the auto-select-family attempt timeout to 2.5 s so a transcontinental IPv4 connect is not cut off mid-handshake - raster-client: 20 s AbortSignal on every query — a stalled request becomes a fast error, never a silent hang; network failures rethrow as RasterUnreachableError naming the endpoint and the undici cause (bare 'fetch failed' says neither) - find: Raster unreachable at the coords step degrades to a one-item playlist with a warning instead of dying — Raster enriches a find, it is not required to build something playable Repro from #97 (FF series URL that hung at 90 s and 150 s) now completes: 35 items, or a fast actionable error when the network is truly down. Suite: 447/447.
feralfile-bot
left a comment
feralfile-bot
left a comment
There was a problem hiding this comment.
Reviewers: 3 (approve: 0, comment: 0, request-changes: 3).
Critical issues
src/utilities/ssh-access.ts:103still sends the SSH access request with barefetch, and its precedingassertFF1CommandCompatibilityprobe defaults to barefetchas well. Either stalled request can leaveff-cli sshwaiting indefinitely, contradicting the PR’s shared-deadline / “no fetch can hang the CLI” guarantee. Route both device requests through the shared timeout mechanism, and add coverage that verifies the SSH request receives a deadline signal. This was independently identified by all three reviewers.
|
Automated human review request for PR #101. Risk zone: red This PR needs targeted human review on the scoped areas below, not a full second pass of the whole PR.
|
Both reviewers caught it: rasterQuery only converted failures from fetch() itself. A body that stalls or terminates after headers rejects in response.text()/response.json(), outside the boundary — so resolveCoords couldn't recognize RasterUnreachableError and never performed its promised single-token fallback. Body consumption now sits inside the boundary: a rejected read on an ok response throws RasterUnreachableError (a 200 whose body can't be read is unusable either way; falling back is right even for server-side garbage). Intentional API-level errors keep their types: an HTTP error stays a status error even when its body is unreadable (best-effort text, never masks the status), and GraphQL errors are unchanged. Tests: mid-stream termination → typed error; 502 with unreadable body → status error, not unreachable; end-to-end find run against a mock server that destroys the body → warns and degrades to a one-item playlist. 450/450.
…arketplace→Raster A Feral File series URL resolved ONE token via ff-marketplace, then asked Raster, which maps the token to its parent artwork — so a single-edition series page (e.g. 36 Points Display Edition) built the sibling collection's 35 tokens instead of the 1 the page describes. Flagged as out-of-scope in #98; this is that fix. Series URLs now route through source-resolver's Feral File site module (series slug → /api/series → /api/artworks?seriesID), exactly like shows already did — one FF resolution path for page-shaped URLs, and find builds precisely the tokens the page describes. Artwork URLs keep the coords→Raster path: a single-token page carries no series intent to preserve. Verified live: the Display Edition series URL now builds 1 item with correct provenance (was 35), in seconds (was minutes of indexer warming for tokens the user never asked about).
Completes the #97 hardening beyond Raster. Every resolver-facing fetch (Art Blocks, Feral File, fxhash, Neort, Objkt, OpenSea, Verse, playlist-source, nft-indexer, feed-fetcher — 15 call sites) now goes through fetchWithTimeout, so a stalled connect or unanswered request is structurally incapable of hanging the CLI. Caller-supplied signals are composed with the deadline via AbortSignal.any, never replaced. The FF indexer batch loop already logged progress — verbose-gated, so a multi-minute warm of unseen tokens rendered as a silent hang in normal runs. getNFTTokenInfoBatch gains an optional onProgress callback; find renders per-batch progress and surfaces the --limit warming hint upfront (it used to appear only after a failure). Suite: 449/449.
…ed deadline All three reviewers caught the gap: ssh-access.ts sent the device request with bare fetch, and assertFF1CommandCompatibility's default probe did too — either could hang `ff-cli ssh` indefinitely, contradicting this PR's no-fetch-can-hang guarantee. Both now route through fetchWithTimeout; injected fetchFn overrides are unchanged. Tests: ssh request and default probe each carry a deadline signal. 454/454.
9ff62bd to
2107193
Compare
|
Addressed in e700727's follow-up commit: ssh-access.ts and assertFF1CommandCompatibility's default probe both route through fetchWithTimeout now (injected fetchFn overrides unchanged), with tests asserting each device request carries a deadline signal. Branch also rebased onto the updated #100/#98 stack. 454/454. |
feralfile-bot
left a comment
feralfile-bot
left a comment
There was a problem hiding this comment.
Reviewers: 3 (approve: 1, comment: 1, request-changes: 1).
- Medium —
findsuppresses the final progress callback (done === total), so a one-batch index—including the recommended--limit 5warm-up—shows noN/M tokens indexed...output, while multi-batch runs never show completion. This was independently identified by two reviewers and defeats the visible-progress goal for the most common small runs. Render the final callback (or emit batch-start progress), and add coverage for single- and multi-batch progress output.
The test called sendSshAccessCommand with an invented argument shape and leaned on the developer's real ~/.config/ff-cli to resolve a device — green on a configured machine, red in CI where the config-resolution early-out returned before any fetch. Now it uses the real signature, writes a temp config and points XDG_CONFIG_HOME at it, asserts the command actually succeeded (so an early-out can't fake a pass), and requires every device fetch — probe and request — to carry a deadline signal.
Two reviewers caught it: suppressing the final callback meant a one-batch index — including the recommended --limit 5 warm-up — printed no progress at all, and multi-batch runs never showed completion. The final N/N line IS the indexing-finished signal; render unconditionally. Coverage: single batch → [1/1]; 12 tokens → [10/12, 12/12]; a throwing progress renderer never breaks the batch. 457/457.
|
Progress finding addressed: the callback renders unconditionally now — the final N/N line is the completion signal, so single-batch runs (including the --limit 5 warm-up) show progress too. Coverage added for single-batch [1/1], multi-batch [10/12, 12/12], and a throwing renderer never breaking the batch. Also fixed this cycle: the ssh deadline test now carries its own temp device config (XDG_CONFIG_HOME), which is what CI red was — the original test leaned on the developer's real ~/.config/ff-cli and asserted nothing when device resolution early-outed. 457/457. |
feralfile-bot
left a comment
feralfile-bot
left a comment
There was a problem hiding this comment.
Reviewers: 3 (approve: 0, comment: 0, request-changes: 3).
Critical issues
- The shared-deadline goal remains incomplete. Several production defaults still use bare
globalThis.fetch:feral-file-artwork.ts,ff1-device.ts,ff1-relayer.ts, andhandoff-client.ts. A stalled artwork lookup, device cast/relayer request, or handoff-broker poll can therefore still hang the CLI. This is a recurring concern: the follow-up correctly covered SSH access and the compatibility probe, but these remaining paths still contradict the PR’s “no fetch can hang” / “everywhere” claim. Route their default implementations throughfetchWithTimeoutwhile retaining injectable fetch functions.
Medium issues
-
find.tssuppresses the callback whendone === total. Since the indexer reports only after a batch completes, a one-batch run (including the suggested--limit 5warm-up) emits noN/N tokens indexed...line, and multi-batch runs omit the final batch. Render the callback unconditionally (or emit an initial progress line). -
fetchWithTimeoutcomposes onlyinit.signal. For aRequestinput that carries its own signal, supplying the timeout signal replaces that cancellation rather than composing it, contrary to the helper’s stated contract. Composeinit.signal ?? input.signalwheninputis aRequest.
Missing tests
- Cover deadlines for the remaining default-fetch paths, especially device casting, relayer delivery, handoff polling, and Feral File artwork resolution.
- Assert progress output for a single-batch run and the final batch of a multi-batch run.
- Assert that a signal attached to a
Requestinput propagates throughfetchWithTimeout.
feralfile-bot
left a comment
feralfile-bot
left a comment
There was a problem hiding this comment.
Reviewers: 3 (approve: 0, request-changes: 3).
Critical issues
- Shared deadlines still do not cover several production fetch defaults, contradicting the PR’s “no fetch can hang the CLI” goal.
sendPlaylistToDevicedefaults to bareglobalThis.fetchand passes it to both the compatibility probe and/api/castrequest (src/utilities/ff1-device.ts:196,221,265); the relayer fallback, handoff broker, and Feral File artwork resolver likewise default to bare fetch (src/utilities/ff1-relayer.ts:42,src/utilities/handoff-client.ts:112,src/utilities/feral-file-artwork.ts:40). A stalled response can therefore still hangplay,find --play, pairing, relayer delivery, or Feral File artwork resolution. Route production defaults throughfetchWithTimeoutwhile preserving injectable test fetches. This was independently identified across all three reports (with the primary device path raised by two).
Medium issues
fetchWithTimeoutpreserves onlyinit.signal; for aRequestinput withoutinit.signal, it replaces the request’s existing abort signal with the deadline (src/utilities/http.ts:17-23). Composeinput.signalas well, so supportedRequestcallers retain cancellation semantics alongside the timeout.
Missing tests
- Add focused coverage for the default
sendPlaylistToDeviceprobe and cast paths, relayer/artwork defaults as applicable, and aRequestinput whose own signal aborts the composed request.
…st signals compose Round-2 findings from all three reviewers, all valid: - ff1-device (probe + /api/cast), ff1-relayer, handoff-client, and feral-file-artwork all defaulted their injectable fetch to bare globalThis.fetch — so play, find --play, pairing, relayer delivery, and FF artwork resolution could still hang. All five defaults now use defaultDeadlineFetch (fetchWithTimeout with the standard deadline); injected test fetches are untouched. - fetchWithTimeout replaced a Request input's own abort signal with the deadline when init.signal was absent (init wins over the request's signal in fetch). The Request's signal is now composed via AbortSignal.any alongside the deadline. Coverage: defaultDeadlineFetch attaches a deadline; artwork resolver and relayer defaults carry one; a Request input's own abort propagates through the composed signal. 461/461.
|
Round 2 addressed: ff1-device (probe and cast), ff1-relayer, handoff-client, and feral-file-artwork production defaults all route through defaultDeadlineFetch now — injected test fetches untouched — and fetchWithTimeout composes a Request input's own signal with the deadline via AbortSignal.any instead of replacing it. Tests cover the defaults and the Request-signal propagation. 461/461. |
feralfile-bot
left a comment
feralfile-bot
left a comment
There was a problem hiding this comment.
Reviewers: 3 (approve: 3, comment: 0, request-changes: 0).
No meaningful findings remain after consolidation. The discussion confirms prior timeout, progress-rendering, test-isolation, and Request-signal composition concerns were addressed in follow-up commits.
2 participants
Stacked on #100 (which stacks on #98) — merge in order. Completes the hardening #98 started.
1. Shared HTTP deadlines (no fetch can hang the CLI)
#98 fixed timeouts for Raster only. The same bare-
fetchhazard existed at 15 more call sites: Art Blocks, Feral File, fxhash, Neort, Objkt, OpenSea, Verse, playlist-source, the FF indexer (3), and the feed fetcher (3). All now route through onefetchWithTimeout(20 s default); a stalled connect or unanswered request is structurally incapable of hanging the CLI rather than being per-call-site discipline. Caller-supplied abort signals are composed with the deadline (AbortSignal.any), never replaced.2. Indexer progress is visible
The FF indexer batch loop always had progress logging —
logger.info, which is verbose-gated, so warming a series of unseen tokens (minutes by design) read as a dead terminal in normal runs. This was most of the perceived "hang" in #97 once the network layer was fixed.getNFTTokenInfoBatchgains an optionalonProgresscallback (best-effort, exceptions swallowed);findrendersN/M tokens indexed...per batch and shows the--limit 5warming hint before indexing large sets (it previously appeared only after a failure).Tests
449/449. New: deadline signal present on every wrapped fetch; caller signal composed-not-replaced (abort propagates through
AbortSignal.any).