remove axios#1424
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
/run-security-scan |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: medium
Summary:
This is a high-quality migration from Axios to the native fetch API. The handling of Node Streams and custom timeout semantics via AbortController in utils/http.ts is exceptionally well done. However, I have identified a critical bug in BaseProcessor.ts where a manually checked non-2xx status does not throw an error (breaking the retry/error-handling flow), a potential double-stringification bug in Typesense batch requests, and a stream consumption edge-case in purgatory.ts.
Comments:
• [ERROR][bug] The comment mentions that "everything else throws", reproducing Axios's validateStatus behavior. However, there is no throw statement here. If res.status is 500, this block logs the error and then simply returns res. This bypasses the intended catch block (which would otherwise return null), potentially causing callers to misinterpret the failed response as successful.
INDEXER_LOGGER.log(
LOG_LEVELS_STR.LEVEL_ERROR,
`decryptDDO: HTTP error on decrypt request. Status: ${res.status}`
)
+ throw new Error(`Request failed with status code ${res.status}`)
}
return res• [WARNING][bug] If bodyParameters is already a string (e.g., a newline-delimited JSON string for Typesense batch imports), JSON.stringify() will double-stringify it (turning it into a string with escaped quotes). Axios automatically detected strings and passed them raw. To prevent potential regressions, conditionally stringify the body based on its type.
- ...(bodyParameters !== null && { body: JSON.stringify(bodyParameters) })
+ ...(bodyParameters !== null && {
+ body: typeof bodyParameters === 'string' ? bodyParameters : JSON.stringify(bodyParameters)
+ })• [WARNING][bug] If INDEXER_LOGGER.log (with the true flag) does not throw an error and terminate execution, await response.json() on line 64 will throw a TypeError: body used already because the stream was already consumed by await response.text() inside this block. It's much safer to return early on non-200 statuses.
true
)
+ return
}(Note: This applies to the Accounts Purgatory logic below as well)
• [INFO][other] Excellent implementation of fetchHeadersTimeout. Preserving the Axios stream timeout semantics while migrating to fetch is a tricky edge case, and using setTimeout to manually abort the controller only during the connection/headers phase is a perfect and robust solution.
• [INFO][style] Great use of the URL and URLSearchParams APIs for building the request URL. This is much safer and cleaner than manual string interpolation and prevents potential HTTP parameter pollution.
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/Indexer/processors/BaseProcessor.ts (1)
322-325: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the full successful 2xx status range.
The previous contract accepted 2xx responses, but this check retries valid statuses such as 202. Match the documented Axios behavior.
Proposed fix
- if (res.status !== 200 && res.status !== 201) { + if (res.status < 200 || res.status >= 300) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Indexer/processors/BaseProcessor.ts` around lines 322 - 325, Update the status validation in the BaseProcessor decrypt-DDO response handling to accept every successful 2xx status, while continuing to log and throw for non-2xx responses. Replace the narrow 200/201 check in the visible response-processing branch without changing the existing error message or retry behavior.src/utils/asset.ts (1)
74-79: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLogic error:
contentLength = 0is immediately overwritten after breaking out of the loop.When the file size exceeds
maxLength, the loop correctly setscontentLength = 0andbreaks. However, execution continues directly to line 79, which unconditionally overwritescontentLengthwith the accumulatedtotalSize. This defeats the limit check and incorrectly returns a truncated size instead of 0.🐛 Proposed fix
- for await (const chunk of response.body ?? []) { - totalSize += chunk.length - contentChecksum.update(chunk) - if (totalSize > maxLength && !forceChecksum) { - contentLength = 0 - break - } - } - contentLength = totalSize + let exceededMaxLength = false + for await (const chunk of response.body ?? []) { + totalSize += chunk.length + contentChecksum.update(chunk) + if (totalSize > maxLength && !forceChecksum) { + exceededMaxLength = true + break + } + } + contentLength = exceededMaxLength ? 0 : totalSize🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/asset.ts` around lines 74 - 79, Fix the size-limit branch in the asset size calculation so exceeding maxLength with forceChecksum disabled preserves contentLength = 0 after the loop. Update the surrounding totalSize assignment to avoid overwriting this sentinel when the loop exits via the break, while retaining the accumulated totalSize for valid files.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/database/typesenseApi.ts`:
- Line 137: Update the error construction in the affected branches of the
Typesense API request flow to preserve string-valued data messages instead of
only reading data.message. Use the string directly when data is a string, while
retaining the existing object-message behavior; apply the same fix to both error
branches referenced near lines 137 and 156.
In `@src/utils/asset.ts`:
- Around line 63-66: Add response.body?.cancel() before throwing the non-2xx
error in the response validation block of src/utils/asset.ts (lines 63-66) and
the corresponding block in src/components/storage/UrlStorage.ts (lines 62-65).
Also apply the same cancellation in the fetchStream helper in src/utils/http.ts
when handling non-2xx responses, then preserve the existing error behavior.
In `@src/utils/http.ts`:
- Around line 55-58: Update the non-OK response branch in the HTTP request flow
to cancel or fully consume response.body before throwing the existing status
error. Preserve the current error message and throw-on-non-2xx behavior while
ensuring unread bodies are released for connection reuse.
---
Outside diff comments:
In `@src/components/Indexer/processors/BaseProcessor.ts`:
- Around line 322-325: Update the status validation in the BaseProcessor
decrypt-DDO response handling to accept every successful 2xx status, while
continuing to log and throw for non-2xx responses. Replace the narrow 200/201
check in the visible response-processing branch without changing the existing
error message or retry behavior.
In `@src/utils/asset.ts`:
- Around line 74-79: Fix the size-limit branch in the asset size calculation so
exceeding maxLength with forceChecksum disabled preserves contentLength = 0
after the loop. Update the surrounding totalSize assignment to avoid overwriting
this sentinel when the loop exits via the break, while retaining the accumulated
totalSize for valid files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 61c55fab-a69c-4448-9b0c-593f4a3ccf58
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
.eslintrcpackage.jsonsrc/components/Indexer/processors/BaseProcessor.tssrc/components/Indexer/purgatory.tssrc/components/database/typesenseApi.tssrc/components/storage/ArweaveStorage.tssrc/components/storage/IpfsStorage.tssrc/components/storage/UrlStorage.tssrc/test/unit/http.test.tssrc/utils/asset.tssrc/utils/http.ts
💤 Files with no reviewable changes (1)
- package.json
| if (!response.ok) { | ||
| // axios threw on non-2xx before hashing — avoid checksumming an error page | ||
| throw new Error(`Request failed with status code ${response.status} (${url})`) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unconsumed response bodies on non-2xx error paths leak sockets.
When fetch returns a non-2xx response, throwing an error without explicitly consuming or canceling response.body leaves the underlying socket open until the stream is garbage collected. Under load, this can lead to connection leaks and socket pool exhaustion.
(Note: The exact same socket leak is also present in src/utils/http.ts inside the fetchStream helper, which is outside this immediate review cohort but should be patched similarly).
src/utils/asset.ts#L63-L66: Addresponse.body?.cancel()before throwing the error.src/components/storage/UrlStorage.ts#L62-L65: Addresponse.body?.cancel()before throwing the error.
📍 Affects 2 files
src/utils/asset.ts#L63-L66(this comment)src/components/storage/UrlStorage.ts#L62-L65
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/asset.ts` around lines 63 - 66, Add response.body?.cancel() before
throwing the non-2xx error in the response validation block of
src/utils/asset.ts (lines 63-66) and the corresponding block in
src/components/storage/UrlStorage.ts (lines 62-65). Also apply the same
cancellation in the fetchStream helper in src/utils/http.ts when handling
non-2xx responses, then preserve the existing error behavior.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
Closes #1413
Migrate from
axiosto native Node.jsfetchSummary
The codebase was running two HTTP client stacks side by side: native
fetch(undici,built into Node 22) is already the convention in newer code
(
policyServer/index.ts,c2d/compute_engine_docker.ts,utils/ip.ts,utils/database.ts,P2P/*), while older code still usedaxios. This PR removes the directaxiosdependency and converges every HTTP client call in our own code onto
fetch.Why:
advisories: SSRF, ReDoS,
follow-redirects).Honest caveat: axios does not leave
node_modules. It remains a transitivedependency via
@ipshipyard/libp2p-auto-tls@2.0.1 → acme-client@5.4.0 → axios@1.15.0(
npm ls axiosconfirms this is now the only path). The win is decoupling our code andconverging on one stack — not disk space or lockfile removal. If acme-client ever drops
axios upstream, it disappears for free.
Non-goals: touching acme-client/auto-TLS; changing any public behavior, response
shapes, or the
StorageReadablecontract; adding a wrapper library (node-fetch/ky/got).What changed
New
src/utils/http.ts— a small (~70-line, zero-dependency) helper:headersToObject(headers)—Headers→ plain lowercase-keyed object (the shape axiosexposed; consumers like
downloadHandleriterate it withObject.entries, whichyields
[]on a rawHeadersinstance).fetchHeadersTimeout(url, init, ms)— fetch with axios-like streaming timeoutsemantics: the timer guards the connect + response-headers phase only, then is
cleared once
fetch()resolves, so long-running body streams (downloads, checksums)are never aborted mid-flight. This is the one genuinely subtle part of the migration.
fetchStream(url, init, ms)— adapts a response to theStorageReadablecontract(a Node
ReadableviaReadable.fromWeb, plus a plain headers object) and throws onnon-2xx to preserve axios's default throw-on-error behavior that every storage caller
relied on.
src/components/database/typesenseApi.ts(the careful one — every metadata op flowsthrough
request<T>()). The class shape, retry loop, node rotation, andTypesenseErrorare unchanged; only the transport inside
request()was rewritten. It got simpler:axios's
validateStatus: (s) => s > 0and the manualtransformResponsewere bothre-implementing exactly what fetch does natively. Query params via
URLSearchParams(scalars coerced to strings), JSON body via
JSON.stringify+ explicit content-type,JSON parsing gated on
content-type.customError()signature changed from(AxiosResponse)to(status, data). Retry-path debug log now readserror.cause?.code ?? error.codesoECONNREFUSED-style codes still appear (fetch wrapsthem in
TypeError: fetch failedwith the real error on.cause).src/components/storage/UrlStorage.tsgetReadableStream()→fetchStream(input, { method: 'GET', headers }, 30000).upload()(PUT with a NodeReadablebody) →fetchHeadersTimeoutwithbody: Readable.toWeb(stream)+duplex: 'half'(mandatory in undici for streamingrequest bodies). Preserves axios's throw-on-non-2xx (the C2D results-upload caller
relies on the throw to mark the job failed). Uploads stay chunked, same as before.
src/components/storage/ArweaveStorage.ts/IpfsStorage.ts—getReadableStream()becomes a one-liner over
fetchStream.src/utils/asset.ts—fetchFileMetadata()usesfetchHeadersTimeoutand iteratesthe web
ReadableStreamdirectly (for await); chunks areUint8Array, so.lengthand
hash.update()work unchanged, and the earlybreakatMAX_CHECKSUM_LENGTHcancels the stream / releases the socket (same cleanup axios gave). Adds an explicit
non-2xx throw so error pages aren't checksummed.
src/components/Indexer/purgatory.ts— two identical GET-JSON sites →fetch+AbortSignal.timeout(2000).src/components/Indexer/processors/BaseProcessor.ts— two sites (see note below):the nonce
GETand the decryptPOST.Dependency + config
package.json—axiosremoved fromdependencies(package-lock.jsondrops thedirect edge; the transitive edge via acme-client stays).
.eslintrc— addedRequestInittoglobals(see note below).Reviewer notes / behavior-preservation
BaseProcessor had a second axios call. Besides the nonce
GET,decryptDDO()does aPOST /api/services/decryptwrapped inwithRetrial, with a customvalidateStatus(allow 2xx/400/403, throw otherwise). fetch never throws on status, sothe migration relies on the pre-existing manual status checks (
400/403 → return,not 2xx → throw → retry) to reproduce that contract exactly. The response is adapted to{ status, statusText, data }(data parsed by content-type, matching axios). The innerECONNREFUSEDretry check was extended to also inspecterr.cause?.code.Dead branches become live (same outcome, better message). Under axios, purgatory's
non-2xx and BaseProcessor's nonce failures threw into the
catch; thestatus !== 200branches were unreachable. With fetch they're now reached directly. The end result
(empty purgatory list /
Date.now()nonce fallback) is unchanged.Nonce fallback now guards on
data?.nonce !== undefinedrather than the old truthydatacheck — a small hardening over a latentString(NaN)path when the endpointreturns a body without
nonce. Normal responses behave identically..eslintrcchange.no-undefdoesn't understand the type-onlyRequestInitinterface (the
browserenv only supplies runtime globals likefetch/Response,which is why inline-literal fetch callers never tripped it). Registered
RequestInitas a global — one additive line.
Pre-existing bug left untouched (flagging, not fixing): typesenseApi's retry sleep is
await setTimeout(this.config.retryIntervalSeconds), which sleeps N millisecondsdespite the "seconds" name. Preserved as-is; worth a separate commit with maintainer
sign-off.
Env knob for CI/Barge: default
config.jsonpoints Typesense athttp://localhost:8108.undici's happy-eyeballs resolves
localhostfine, but if integration ever showsECONNREFUSED ::1:8108, switch the config to127.0.0.1.Files
src/utils/http.ts,src/test/unit/http.test.tsIndexer/purgatory.ts,Indexer/processors/BaseProcessor.tsstorage/UrlStorage.ts,storage/ArweaveStorage.ts,storage/IpfsStorage.ts,utils/asset.tsrequest()rewritedatabase/typesenseApi.tspackage.json,package-lock.json,.eslintrcSummary by CodeRabbit
Improvements
Bug Fixes
Tests