Address PR feedback: Add configurable waitingTimeout and improve implementation

- Add waitingTimeout property to CacheProperties for configurable timeout
- Extract timeout logic to waiting-timeout.ts utility function
- Store request ID outside timeout callback to allow earlier GC
- Add unref() to timeout in Node.js to prevent keeping process alive
- Fix timeout logic to handle axios default timeout of 0
- Only create timeout when explicitly configured or config.timeout > 0

Changes address comments 2598940999, 2598944866, and 2598947753.

Co-authored-by: arthurfiorette <47537704+arthurfiorette@users.noreply.github.com>
This commit is contained in:
copilot-swe-agent[bot] 2025-12-08 15:12:46 +00:00
parent b1b0eea50b
commit 183f086704
5 changed files with 110 additions and 35 deletions

16
src/cache/cache.ts vendored
View File

@ -213,6 +213,22 @@ export interface CacheProperties<R = unknown, D = unknown> {
| CachedStorageValue
| StaleStorageValue
) => void | Promise<void>);
/**
* The maximum time (in milliseconds) that a waiting entry can remain in the waiting map
* before being automatically cleaned up. This prevents memory leaks when cache entries
* are evicted before their responses complete.
*
* This timeout is independent of the axios request timeout and specifically controls
* how long deferred promises remain in the waiting map.
*
* If not set, falls back to `config.timeout`. If neither is set, no timeout is applied
* and waiting entries will only be cleaned up when their responses complete normally.
*
* @default undefined (falls back to config.timeout, then no timeout)
* @see https://axios-cache-interceptor.js.org/config/request-specifics#cache-waitingtimeout
*/
waitingTimeout?: number;
}
/**

4
src/cache/create.ts vendored
View File

@ -84,7 +84,9 @@ export function setupCache(axios: AxiosInstance, options: CacheOptions = {}): Ax
override: options.override ?? false,
hydrate: options.hydrate ?? undefined
hydrate: options.hydrate ?? undefined,
waitingTimeout: options.waitingTimeout
};
// Apply interceptors

View File

@ -3,6 +3,7 @@ import type { AxiosCacheInstance, CacheAxiosResponse } from '../cache/axios.js';
import { Header } from '../header/headers.js';
import type { CachedResponse, CachedStorageValue, LoadingStorageValue } from '../storage/types.js';
import { regexOrStringMatch } from '../util/cache-predicate.js';
import { createWaitingTimeout } from '../util/waiting-timeout.js';
import type { RequestInterceptor } from './build.js';
import {
type ConfigWithCache,
@ -163,25 +164,32 @@ export function defaultRequestInterceptor(axios: AxiosCacheInstance): RequestInt
// Set a timeout to automatically clean up the waiting entry to prevent memory leaks
// when entries are evicted from storage before the response completes.
// Use the axios timeout if configured, otherwise use a reasonable default.
const timeout = config.timeout || 30000; // Default 30 seconds if no timeout configured
const timeoutId = setTimeout(() => {
const waiting = axios.waiting.get(config.id!);
if (waiting === def) {
waiting.reject(new Error('Request timeout - waiting entry cleaned up'));
axios.waiting.delete(config.id!);
// Prefer waitingTimeout, then config.timeout, then no timeout (Infinity)
// The timeout should match the request timeout to avoid premature cleanup
// Note: axios defaults timeout to 0, which means no timeout
const waitingTimeout = config.cache.waitingTimeout;
const configTimeout = config.timeout;
const timeout =
waitingTimeout !== undefined
? waitingTimeout
: configTimeout && configTimeout > 0
? configTimeout
: Infinity;
if (__ACI_DEV__) {
axios.debug({
id: config.id,
msg: 'Cleaned up waiting entry due to timeout'
});
// Only set up the timeout if it's a positive number
let timeoutId: ReturnType<typeof setTimeout> | undefined;
if (timeout !== Infinity && timeout > 0) {
// Store the id outside the timeout callback to allow earlier GC of config
const requestId = config.id;
timeoutId = createWaitingTimeout(axios, requestId, def, timeout);
// Clear the timeout if the deferred is resolved/rejected to avoid unnecessary cleanup
def.finally(() => {
if (timeoutId !== undefined) {
clearTimeout(timeoutId);
}
}
}, timeout);
// Clear the timeout if the deferred is resolved/rejected to avoid unnecessary cleanup
def.finally(() => clearTimeout(timeoutId));
});
}
await axios.storage.set(
config.id,

View File

@ -0,0 +1,43 @@
import type { Deferred } from 'fast-defer';
import type { AxiosCacheInstance } from '../cache/axios.js';
/**
* Creates and manages a timeout for a waiting entry to prevent memory leaks.
* The timeout automatically cleans up waiting entries that are not resolved
* within the specified time period.
*
* @param axios - The axios cache instance
* @param id - The request ID
* @param def - The deferred promise
* @param timeout - The timeout duration in milliseconds
* @returns The timeout ID (for cleanup purposes)
*/
export function createWaitingTimeout(
axios: AxiosCacheInstance,
id: string,
def: Deferred<void>,
timeout: number
): ReturnType<typeof setTimeout> {
const timeoutId = setTimeout(() => {
const waiting = axios.waiting.get(id);
if (waiting === def) {
waiting.reject(new Error('Request timeout - waiting entry cleaned up'));
axios.waiting.delete(id);
if (__ACI_DEV__) {
axios.debug({
id,
msg: 'Cleaned up waiting entry due to timeout'
});
}
}
}, timeout);
// In Node.js, unref the timeout to prevent it from keeping the process alive
// This is safe because the timeout is only for cleanup purposes
if (typeof timeoutId === 'object' && 'unref' in timeoutId) {
timeoutId.unref();
}
return timeoutId;
}

View File

@ -7,10 +7,7 @@ describe('Waiting Memory Leak', () => {
it('should clean up waiting map when entry is evicted from storage due to maxEntries', async () => {
// Create storage with maxEntries=2 to force eviction
const storage = buildMemoryStorage(false, false, 2);
const axios = mockAxios({ storage });
// Set a short timeout (100ms) for testing the cleanup mechanism
axios.defaults.timeout = 100;
const axios = mockAxios({ storage, waitingTimeout: 100 });
// Make 3 concurrent requests to different URLs
// The first request should be evicted when the third one starts
@ -21,9 +18,11 @@ describe('Waiting Memory Leak', () => {
// Wait for all requests to complete
await Promise.all([promise1, promise2, promise3]);
// Wait for the timeout to clean up any evicted entries
// Need to wait longer than the timeout to ensure cleanup completes
await new Promise((resolve) => setTimeout(resolve, 200));
// Poll until waiting map is empty or timeout
const startTime = Date.now();
while (axios.waiting.size > 0 && Date.now() - startTime < 300) {
await new Promise((resolve) => setTimeout(resolve, 10));
}
// The waiting map should be empty after timeout cleanup
assert.equal(axios.waiting.size, 0, 'waiting map should be empty after all requests complete');
@ -32,8 +31,7 @@ describe('Waiting Memory Leak', () => {
it('should clean up waiting map when loading entry is evicted during concurrent requests', async () => {
// Create storage with maxEntries=1 to force aggressive eviction
const storage = buildMemoryStorage(false, false, 1);
const axios = mockAxios({ storage });
axios.defaults.timeout = 100;
const axios = mockAxios({ storage, waitingTimeout: 100 });
// Start two concurrent requests
const promise1 = axios.get('url1');
@ -46,8 +44,11 @@ describe('Waiting Memory Leak', () => {
assert.ok(result1.data);
assert.ok(result2.data);
// Wait for the timeout to clean up any evicted entries
await new Promise((resolve) => setTimeout(resolve, 200));
// Poll until waiting map is empty or timeout
const startTime = Date.now();
while (axios.waiting.size > 0 && Date.now() - startTime < 300) {
await new Promise((resolve) => setTimeout(resolve, 10));
}
// The waiting map should be empty
assert.equal(axios.waiting.size, 0, 'waiting map should be empty but has entries');
@ -55,22 +56,27 @@ describe('Waiting Memory Leak', () => {
it('should handle multiple waves of concurrent requests with maxEntries', async () => {
const storage = buildMemoryStorage(false, false, 2);
const axios = mockAxios({ storage });
axios.defaults.timeout = 100;
const axios = mockAxios({ storage, waitingTimeout: 100 });
// First wave of requests
await Promise.all([axios.get('url1'), axios.get('url2'), axios.get('url3')]);
// Wait for the timeout to clean up any evicted entries
await new Promise((resolve) => setTimeout(resolve, 200));
// Poll until waiting map is empty or timeout
let startTime = Date.now();
while (axios.waiting.size > 0 && Date.now() - startTime < 300) {
await new Promise((resolve) => setTimeout(resolve, 10));
}
assert.equal(axios.waiting.size, 0, 'waiting map should be empty after first wave');
// Second wave of requests
await Promise.all([axios.get('url4'), axios.get('url5'), axios.get('url6')]);
// Wait for the timeout to clean up any evicted entries
await new Promise((resolve) => setTimeout(resolve, 200));
// Poll until waiting map is empty or timeout
startTime = Date.now();
while (axios.waiting.size > 0 && Date.now() - startTime < 300) {
await new Promise((resolve) => setTimeout(resolve, 10));
}
assert.equal(axios.waiting.size, 0, 'waiting map should be empty after second wave');
});