fix: prevent caching error responses in fetchWithCache#8070
fix: prevent caching error responses in fetchWithCache#8070mldangelo merged 3 commits intopromptfoo:mainfrom
Conversation
![promptfoo-scanner[bot]](https://avatars.githubusercontent.com/in/2172945?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 All Clear
Reviewed the small change in src/cache.ts to avoid caching JSON responses that include an error field. Traced the fetch/cache path and confirmed no prompts, LLM tool capabilities, or execution sinks were modified. This PR narrows caching behavior and does not introduce any LLM security risks.
Minimum severity threshold: 🟡 Medium | To re-scan after changes, comment @promptfoo-scanner
Learn more
📝 WalkthroughWalkthroughThe Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cache.ts`:
- Around line 247-251: The branch in the fetch/caching flow uses closure-local
errorResponse to return non-cacheable JSON payloads, which fails under
cache.wrap() deduplication because concurrent waiters won't set that variable;
instead, make the producer signal the non-cacheable response by throwing a
specific sentinel (e.g., NonCacheableResponseError) when parsedData?.error is
detected in the block that currently sets errorResponse and returns, then catch
that sentinel around the cache.wrap() call and extract/return the payload to
callers; update the logger.debug message in the parsedData?.error branch and
ensure other errors still propagate normally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c6dd7b2a-00fb-448f-bb5a-dc1855b2ff7b
📒 Files selected for processing (1)
src/cache.ts
src/cache.ts
Outdated
| // Don't cache if the parsed data contains an error | ||
| if (format === 'json' && parsedData?.error) { | ||
| logger.debug(`Not caching ${url} because it contains an 'error' key: ${parsedData.error}`); | ||
| return data; | ||
| errorResponse = data; | ||
| return; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does cache-manager's wrap()deduplicate concurrent calls for the same key, and if the wrapped function returnsundefined, do waiting callers receive that same undefined result?
💡 Result:
Yes—cache-manager’s wrap() deduplicates concurrent calls for the same key within the same process/instance: it wraps the whole “get-from-cache → maybe call fn → set” flow in coalesceAsync(${cacheId}::${key}, ...), so concurrent callers share the same in-flight Promise and only one execution of fnc() happens at a time for that key. [1]
If fnc() resolves to undefined, all waiting callers will receive that same undefined, because they’re awaiting the same coalesced Promise result. [1]
However, wrap() treats undefined as a cache miss (if (value === undefined) { const result = await fnc(); ... }), so even if the underlying Keyv store can technically persist an undefined payload, wrap() will not consider it a cached hit on later calls—subsequent wrap() calls will run fnc() again. [1][2]
Sources:
[1] cache-manager wrap() implementation (uses coalesceAsync, and checks value === undefined) https://github.com/jaredwray/cacheable/blob/main/packages/cache-manager/src/index.ts
[2] Keyv get() miss semantics (miss returns undefined) https://keyv.org/docs/keyv/
🏁 Script executed:
# Get context around the changed lines and the return statement at line 268
head -n 280 src/cache.ts | tail -n 50Repository: promptfoo/promptfoo
Length of output: 1621
🏁 Script executed:
# Look for the fetchWithCache function and the cache.wrap call
rg -A 50 "async function fetchWithCache" src/cache.tsRepository: promptfoo/promptfoo
Length of output: 1803
🏁 Script executed:
# Find the parseResult function to see if it handles null/undefined
rg -B 3 -A 10 "function parseResult|const parseResult" src/cache.tsRepository: promptfoo/promptfoo
Length of output: 45
Avoid using errorResponse as a wrap() side channel.
Because cache.wrap() deduplicates concurrent calls for the same key, concurrent callers waiting on the same in-flight request do not execute the producer and thus never assign their own errorResponse. When this branch returns undefined, waiting callers reach line 268 with both cachedResponse and errorResponse unset, causing JSON.parse(null) to throw instead of returning the error payload to downstream code. Propagate the non-cacheable response through a shared mechanism (e.g., throw a sentinel caught around cache.wrap()) rather than via closure-local state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cache.ts` around lines 247 - 251, The branch in the fetch/caching flow
uses closure-local errorResponse to return non-cacheable JSON payloads, which
fails under cache.wrap() deduplication because concurrent waiters won't set that
variable; instead, make the producer signal the non-cacheable response by
throwing a specific sentinel (e.g., NonCacheableResponseError) when
parsedData?.error is detected in the block that currently sets errorResponse and
returns, then catch that sentinel around the cache.wrap() call and
extract/return the payload to callers; update the logger.debug message in the
parsedData?.error branch and ensure other errors still propagate normally.
|
Thank you again @aerosta! |
Thanks @mldangelo, happy to contribute. |
3 participants
Summary
fetchWithCachecaches HTTP 200 responses that contain an error body such as{"error": "rate limit exceeded"}.That path is meant to skip caching, but it returns
data, socache.wrap()stores the error response and poisons the cache for up to 14 days.The
!response.okbranch already avoids this by settingerrorResponseand returningundefined. This change makes the error-key path behave the same way.Fix
errorResponseundefinedinstead ofdataTests
Local Node version is outside the repo's supported range, so final validation should come from CI.