fix(downloads): stop cold-cache downloads failing silently, and fetch gateway chunks in parallel - #812
fix(downloads): stop cold-cache downloads failing silently, and fetch gateway chunks in parallel#812EmilFattakhov wants to merge 5 commits into
Conversation
An uncached object is reconstructed from the DSN before its first byte can be written, which the codebase itself documents as taking 20+ minutes. Every layer between the user and that work gave up first, and none of them could say why — so a slow-but-healthy retrieval was indistinguishable from a file that no longer existed. nginx was the visible failure. Only `location /s3` ever set a proxy timeout; /api, /file and /folder inherited the 60s default, and the browser's download URL is .../api — so a cold download was cut with a 504 roughly a minute in. These are idle timeouts between upstream reads, so healthy streaming downloads were never affected and never revealed the problem. Above that, the client gave up at 10 minutes (60 polls x 10s) and reported "Download preparation timed out" against a path documented as taking twice that. Any error checking availability silently disabled preparation and fell through to the direct fetch — the request that 504s. Anonymous users could never check status or queue a retrieval at all, because the client demanded a session for an endpoint the server serves unauthenticated. Nothing reported progress, either. `Downloading` was never written, so a running reconstruction sat on `Pending` for its whole life and the badge — which only renders a percentage for `Downloading` — showed no movement for twenty minutes. "Bring to Cache" queued the work, claimed it was done, and never mentioned it again. Changes: - nginx: 1800s read/send timeouts on every download-serving location, including the gateway's own reconstruct-on-demand route. - Status endpoint reports an in-flight reconstruction and its progress. `status` keeps its two values, so existing clients and the SDK are unaffected; `reconstruction` is additive. - Repeat requests join the run already in flight instead of queueing a competing full pull of the same object. - A running reconstruction reports `Downloading`, which also un-sticks a row a previous attempt left as `Failed`. - Client waits on server state rather than a fixed count, shows retrieved bytes and elapsed time, and treats "still working" as a background hand-off rather than an error. Bulk keeps a short bound so one slow item cannot hold up the batch. - Memory tier decides from the known size instead of assembling the whole object in the worker's heap only for lru-cache to refuse it. Also fixes two crashes in the download UI: BigInt division by zero in the status badge took down the Cached Downloads dialog for any row with a zero size, and the sweep meant to dismiss stale rows used `return` where it meant `continue` while treating one NotCached read as proof a completed download was gone — deleting the user's only record of it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
FileGateway.getFile returns a Readable that pulls exactly one chunk per read(), strictly in series. For the 1.28 GB object in the current backlog that is 19,649 sequential HTTP round-trips to the gateway — at 50ms each, about sixteen minutes before the first byte reaches the user; at 200ms, over an hour. This is most of what "reconstruction is extremely slow" means. The database path has always fetched 100 chunks at a time via composeNodesDataAsFileReadable; only the gateway path was serial. Composes the file from the same per-chunk endpoint the SDK's own getChunkedFile uses (/files/:cid/partial?chunk=i), with FILES_GATEWAY_CHUNK_CONCURRENCY requests in flight. Chunk order is preserved and the end of the file is still decided by the gateway's 204, so the bytes are identical; only the request pattern changes. The endpoint is reimplemented in our FileGateway wrapper because @autonomys/auto-files exposes it only behind getFile. It is deliberately not built on FileGateway.getNode: that route returns a decoded PBNode via res.json, so its bytes are JSON text rather than chunk payload and nothing IPLD-decodes them on the way back. Chunks that a full consumer cannot accept mid-batch are held rather than refetched, so backpressure does not cost a second pull of up to CHUNK_CONCURRENCY chunks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
✅ Deploy Preview for auto-drive-storage ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…ams that die Fixes found by an independent verification pass over this branch. The parallel fetcher's read() was async. Node clears its own re-entrancy guard on the first push(), so a read() that pushes and then awaits is called again while still suspended, and both calls claim the same chunk range. Armed by a chunk payload just under the 64 KiB high-water mark (the gateway's is 65,066) plus any request latency, so it fired on every cold retrieval: 37 requests for a 21-request file, over half the chunks fetched twice. The duplicate is dropped if it lands after end-of-stream and emitted as file content if it lands before — a download longer than the file it claims to be, under a content address, cached for everyone behind it. read() is now synchronous and starts a single-flight pump that drives itself rather than relying on the read() it displaced. Composing every file per chunk also bypassed the gateway's single streaming response for files it already holds, turning a cached 1.28 GB read into ~19,650 requests, each a fresh DAG walk gateway-side. Cached files keep the one request; only the cold path fans out. Per-chunk retries are back too, matching what the SDK wraps its own chunk fetch in — one transient failure out of ~19,650 otherwise kills the retrieval. forkStream is source.pipe(fork([a, b])), and pipe() does not forward errors, so a source that died mid-file left both branches open with neither end nor error: the response never completed and the client waited out the proxy timeout, which this branch raises to 30 minutes. Branches are now failed when their source fails, each with its own listener first, since an error with no listener is an uncaught exception rather than a failed request. An async download row now has to have been stamped recently to count as in flight, and the worker stamps it every 30s. Pending and Downloading otherwise mean "nothing has touched this row", which is equally true of a worker that died — and since the status query is not scoped to a user and the client disables its own request while a reconstruction runs, one abandoned row took Bring to Cache away from everyone asking about that cid. Also sets the read timeout on the staging gateway's location, missed by the earlier nginx change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit fd26bda. Configure here.
| Array.from({ length: batchSize }, (_, offset) => | ||
| fetchChunk(batchStart + offset), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Parallel fetches exceed gateway connection limit
High Severity
The cold-path fetcher opens up to FILES_GATEWAY_CHUNK_CONCURRENCY (default 100) simultaneous connections to the gateway, but both gateway nginx configs still apply limit_conn addr 5 on that same location. Extra connections are rejected with 503, Promise.all fails the batch, and the retrieval dies. Timed-out fetch calls are also not aborted, so they keep occupying those five slots through the new 1800s proxy timeout.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit fd26bda. Configure here.
There was a problem hiding this comment.
Correcting my earlier reply — I had this wrong. I claimed FILES_GATEWAY_URL addresses the gateway process directly and so the vhost's limit_conn addr 5 was out of the path. Production says otherwise: it is https://gateway.mainnet.autonomys.xyz, which resolves to a Cloudflare edge. The fan-out does cross the origin's limiter. Corrected in 0924782.
The finding is now plausible but still unproven, and not for the reason it gives. Cloudflare is between us and the origin, which changes both halves of the arithmetic: limit_conn_zone $binary_remote_addr at the origin keys on the edge's IP, not this backend's, unless the gateway host sets real_ip_header CF-Connecting-IP; and the edge pools its own origin connections, so 100 requests from here need not become 100 connections there. Whether 100-wide trips a 5-connection limit is therefore a question about Cloudflare's connection reuse, which no amount of reading either config can answer. Measuring it against the real URL from the real container is the only way to settle it, and that is in progress.
The abort half stands and is fixed in 146cdab — withTimeout always accepted an AbortController and nothing passed one, so a timed-out chunk kept its connection until the gateway closed it. That is worse under a connection limit, not better, so it wanted fixing either way. Controller per attempt rather than per chunk, since a shared one aborts the retries the moment it fires.
Holding the merge until the concurrency question is measured.
| // DAG walk from the root gateway-side, for bytes already on its disk. | ||
| if (await isFileCachedOnGateway(cid)) { | ||
| logger.debug('Fetching cached file from gateway cid=%s', cid) | ||
| return fetchGatewayFile(cid) |
There was a problem hiding this comment.
Cached gateway fetch lost its timeout
Medium Severity
The cached path used to wrap FileGateway.getFile in withTimeout (60s by default). It now calls isFileCachedOnGateway and fetchGatewayFile with no timeout. A hung gateway never rejects, so download() never returns, the async-download heartbeat never starts, and the client waits until the 30-minute proxy timeout.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fd26bda. Configure here.
There was a problem hiding this comment.
Confirmed and fixed in 146cdab. main wrapped FileGateway.getFile in withTimeout(GATEWAY_TIMEOUT_MS) and restoring the cached path dropped it.
Worth stating why it mattered more than a lost 60s bound usually does: this call sits upstream of everything that would otherwise notice. No status row has been written, the inactivity timer is not armed, and the heartbeat has not started — so a gateway that accepts the connection and then says nothing had nothing at any layer to time it out, and the client waited out the full 1800s proxy timeout this PR introduces.
Both the cache-status check and the fetch are bounded again. The timeout covers acquiring the stream, not the transfer over it, which is the same semantic main had — a slow but healthy download is unaffected. A regression test hangs fetchGatewayFile and asserts the call rejects; against the previous commit it hits Jest's own 5s timeout instead.
…iven up on Restoring the gateway's single-request path for cached files dropped the timeout main had around FileGateway.getFile. A gateway that accepts the connection and then says nothing never rejects, so download() never returns — and this call sits upstream of everything that reports a download as running, so no heartbeat, no inactivity timer and no status row exists yet to catch it. The client waits out the 1800s proxy timeout. Both the cache-status check and the fetch are bounded again. Chunk requests now carry an AbortSignal, so one the fetcher has stopped waiting for hands its connection back instead of holding it for as long as the gateway will. A controller per attempt rather than per chunk: timing out is what arms it, so a shared one would abort the retries the moment it fired. Records why the chunk concurrency may exceed the gateway vhost's limit_conn addr 5 — FILES_GATEWAY_URL addresses the gateway process directly, so that limiter is not in this path. Pointed at the public hostname instead, 95 of every 100 chunk requests would 503, and retries would not clear it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous note claimed FILES_GATEWAY_URL addresses the gateway process directly, so the public vhost's limit_conn addr 5 could not apply. Production says otherwise: the value is the public hostname, and it resolves to a CDN edge, so the fan-out does cross the origin's limiter. Whether it trips is genuinely open — the limiter counts the edge's connections to the origin, not ours to the edge — so the note now says what is known and what to reach for if cold downloads start failing, rather than asserting a clearance that was never verified. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>


Why
An uncached object is reconstructed from the DSN before its first byte can be written. The codebase itself puts that at 20+ minutes for a large archived file (
FilePreview/index.tsx:139). Every layer between the user and that work gave up first, and none of them could say why — so a slow-but-healthy retrieval was indistinguishable from a file that no longer existed. That is the "Bring to Cache / spinner crashes silently" report.The timeout stack before this PR:
/api,/file,/folderFailedlocation /s3was the only block with a proxy timeout. The browser's download URL ishttps://public.auto-drive.autonomys.xyz/api, so the actual file transfer ran under the 60s default. Because these are idle timeouts between upstream reads, healthy streaming downloads never tripped them — which is why this stayed invisible.What changed
nginx — 1800s
proxy_read_timeout/proxy_send_timeouton every download-serving location across all four configs, including both gateways' reconstruct-on-demand routes.location /s3is left at its existing 600s: that one was set deliberately, and raising it is a separate call from replacing a default nobody chose.Server-side truth —
GET /downloads/:cid/statusnow reports an in-flight reconstruction and its byte progress.statuskeeps its two values so existing clients and the SDK are unaffected;reconstructionis additive. Previouslynot-cachedwas the only thing the endpoint could say, and it said the same thing whether a pull was twelve minutes in or had never started.Liveness, not just state — a row only counts as an in-flight reconstruction if it has been stamped within
ASYNC_DOWNLOAD_STALE_AFTER_MS(5 min), and the running worker stamps it everyASYNC_DOWNLOAD_HEARTBEAT_MS(30s). Without both,PendingandDownloadingmean "nothing has changed this row since it was created", which is equally true of a worker that died. That matters here because the status query is deliberately not scoped to a user and the client disables its own request while a reconstruction is running: one abandoned row would take "Bring to Cache" away from everyone asking about that cid, with only its owner able to dismiss it. The heartbeat is what lets the window be 5 minutes instead of longer than a cold retrieval's time-to-first-byte.Deduplication — a user's repeat clicks join their run already in flight instead of queueing a competing full pull. Scoped to
(cid, provider, user), because the row is that user's own record of the request. Two users asking for the same cid still get a row each — they share the cache, so the second is cheap, and the status endpoint is what tells each of them the object is already being fetched. Still a check-then-insert with no unique index, so two genuinely simultaneous clicks can both insert; that needs a partial unique index and is not in this PR.Real progress — nothing ever wrote
AsyncDownloadStatus.Downloading, so a running job sat onPendingfor its whole life, and the badge only renders a percentage forDownloading. Setting it also un-sticks a row a previous attempt left asFailed.Client — waits on server state instead of a fixed poll count (2h backstop), shows retrieved bytes and elapsed time, and treats "still working" as a background hand-off rather than an error. Availability-check failures no longer silently disable preparation and fall through to the direct fetch — the request that 504s. Anonymous users can now check status; the route was always unauthenticated, only the client demanded a session. Bulk downloads keep a 3-minute bound so one slow item cannot hold up the batch.
Parallel gateway retrieval — cold path only.
FileGateway.getFilestreams a file the gateway already holds in one response, and only falls back to composing it one chunk perread(), strictly in series, on a cache miss. That serial fallback is 19,649 sequential round-trips for the 1.28 GB object in the backlog: at 50ms each, ~16 minutes before the first byte. Uncached retrievals now use the same per-chunk endpoint the SDK's owngetChunkedFileuses, withFILES_GATEWAY_CHUNK_CONCURRENCY(default 100) requests in flight andFILES_GATEWAY_CHUNK_RETRIES(default 3) attempts each, every request carrying an abort signal so one that times out hands its connection back rather than holding it. Cached ones keep the single streaming request, still bounded byFILES_GATEWAY_FETCH_TIMEOUT_MS— composing those per chunk would be ~19,650 requests, each a fresh DAG walk gateway-side, for bytes already on its disk. Chunk order is preserved and the end is still decided by the gateway's 204, so the bytes are identical; only the request pattern changes.Stream failures reach the caller.
forkStreamissource.pipe(fork([a, b])), andpipe()does not forward errors, so a source that died mid-file left both branches open with neitherendnorerror. The response never completed and the client waited out the proxy timeout — which this PR raises to 30 minutes, so the same bug got 30× worse. Branches are now failed when their source fails, and each carries its own listener first, since anerrorwith no listener is an uncaught exception rather than a failed request.Memory tier — decides from the known size instead of assembling the whole object in the worker's heap only for lru-cache to refuse it (
maxEntrySizedefaults tomaxSize, so any file above the 1 GB cap cost its full size in RAM to produce a guaranteed miss). Files under the cap are unaffected and still buffer as before.Two crashes fixed in passing
AsyncStatusBadgethrew aRangeErrorout of render, taking down the Cached Downloads dialog for any row with a zero size.returnwhere it meantcontinue, bailing at the first non-completed row — and treated a singleNotCachedread as proof a completed download was gone, deleting the user's only record of it. Removed rather than patched: nothing there needed to mutate server state just to display it.Changes since the first push
An independent verification pass over the branch caught a defect in the parallel fetcher and three regressions, all fixed here. Worth reading if you reviewed the earlier commits.
The fetcher's
read()wasasync. Node clears its own re-entrancy guard (state.reading) on the firstpush(), so an asyncread()that pushes and then awaits is called again while it is still suspended, and both calls read the same cursor and request the same range. Two conditions arm it and both hold on this path: a chunk payload just under the 64 KiB high-water mark (the gateway's is 65,066), so a single push leaves room and returnstrue; and any latency on the request, so the second call lands inside the await. Measured against the previous implementation: 37 requests for a 21-request file, over half the chunks fetched twice. Where the duplicate lands depends on who wins the race — after end-of-stream it is dropped, before it the chunk is emitted a second time as file content, which is a download longer than the file it claims to be, under a content address, written into the cache for everyone behind it. One measured shape (100 chunks, consumer slower than the gateway — i.e. a browser on a real connection) emitted 8 duplicated chunks.read()is now synchronous and starts a single-flight pump that keeps going by itself rather than relying on theread()it displaced, and claims its range before awaiting.The other three: the cached single-request path was being bypassed for every file; per-chunk retries were dropped (the SDK wraps its own chunk fetch in three, and one transient failure out of ~19,650 requests otherwise kills the retrieval); and the
forkStreamerror gap above. Plus the staging gateway'slocation /had been missed by the nginx change.A second round then caught that restoring the cached path had dropped the
withTimeoutmainhad aroundFileGateway.getFile, which matters more than a lost 60s bound usually does — that call is upstream of the status row, the inactivity timer and the heartbeat, so nothing at any layer was left to notice a gateway that accepts a connection and then says nothing. Both it and the cache-status check are bounded again. Chunk requests also now carry an abort signal:withTimeoutalways took anAbortControllerand nothing was passing one, so a timed-out chunk kept running, kept buffering and kept its connection. The controller is per attempt, not per chunk — a shared one aborts the retries the moment it fires.Reviewer notes
limit_conn addr 5. Right while reconstruction is slow; once the parallel fetcher is live in prod the honest number is probably much lower, and I would revisit rather than leave 30 minutes there permanently.fetchFileChunkis deliberately not built onFileGateway.getNode. That route returns a decodedPBNodeviares.json, so its bytes are JSON text rather than chunk payload, and nothing IPLD-decodes them on the way back. Which leads to:retrieveFileByteRangecomposes archived objects throughFileGatewayObjectFetcher.fetchNode— the same/nodes/:cidroute — so ranged reads on an archived, uncached object serve JSON text instead of file bytes. Traced end to end through the gateway'sres.json(node); it only bites on a cache miss, which is likely why it went unnoticed. Left alone rather than changing a path that could not be tested here. The fix is small now thatfetchFileChunkexists.limit_conn addr 5on purpose. That limiter guards the public hostname;FILES_GATEWAY_URLaddresses the gateway process directly, so backend chunk requests are never counted against the zone. Worth knowing before anyone repoints that variable at the public hostname — 95 of every 100 requests would 503, and because the limit is not transient the retries would burn their backoff and still fail. Noted next to the config value too./s3at 600s.Testing
yarn backend test— 56 suites / 873 tests, all passing.yarn backend lint,yarn frontend lint,tsc --noEmiton the frontend — clean.Unrelated but worth knowing: the backend test script uses
--forceExit, which kills Jest before Testcontainers' reaper runs. Repeated local full-suite runs leak containers until new ones cannot start — every suite then fails withglobalSetup ... "Server startup complete" not received after 30000ms. Worth a separate look at whether the flag is still needed.🤖 Generated with Claude Code