mirror of
https://github.com/MichMich/MagicMirror.git
synced 2026-07-22 02:36:42 -07:00
fix(http-fetcher): fall back to reloadInterval after retries exhausted (#4113)
As reported in #4109, the weather module retries much more frequently than expected after network errors. #4092 already fixed the main cause (duplicate fetchers), but the backoff logic in `HTTPFetcher` still has a gap: once retries are exhausted, `calculateBackoffDelay` keeps returning a short fixed delay (60s) instead of falling back to `reloadInterval`. The same problem existed for 5xx errors, where the delay grew to 8× the configured interval. Inspired by #4110 (thanks @CodeLine9), this PR makes both error paths fall back to `reloadInterval` after retries are exhausted. I also simplified the catch block, extracted a `#shortenUrl()` helper for log messages, and added tests for the backoff progression.
This commit is contained in:
committed by
GitHub
parent
3f2a0302eb
commit
7e1286257c
+40
-38
@@ -183,6 +183,19 @@ class HTTPFetcher extends EventEmitter {
|
||||
return null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns a shortened version of the URL for log messages.
|
||||
* @returns {string} Shortened URL
|
||||
*/
|
||||
#shortenUrl () {
|
||||
try {
|
||||
const urlObj = new URL(this.url);
|
||||
return `${urlObj.origin}${urlObj.pathname}${urlObj.search.length > 50 ? "?..." : urlObj.search}`;
|
||||
} catch {
|
||||
return this.url;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Determines the retry delay for a non-ok response
|
||||
* @param {Response} response - The fetch Response object
|
||||
@@ -198,28 +211,35 @@ class HTTPFetcher extends EventEmitter {
|
||||
errorType = "AUTH_FAILURE";
|
||||
delay = Math.max(this.reloadInterval * 5, THIRTY_MINUTES);
|
||||
message = `Authentication failed (${status}). Check your API key. Waiting ${Math.round(delay / 60000)} minutes before retry.`;
|
||||
Log.error(`${this.logContext}${this.url} - ${message}`);
|
||||
Log.error(`${this.logContext}${this.#shortenUrl()} - ${message}`);
|
||||
} else if (status === 429) {
|
||||
errorType = "RATE_LIMITED";
|
||||
const retryAfter = response.headers.get("retry-after");
|
||||
const parsed = retryAfter ? this.#parseRetryAfter(retryAfter) : null;
|
||||
delay = parsed !== null ? Math.max(parsed, this.reloadInterval) : Math.max(this.reloadInterval * 2, FIFTEEN_MINUTES);
|
||||
message = `Rate limited (429). Retrying in ${Math.round(delay / 60000)} minutes.`;
|
||||
Log.warn(`${this.logContext}${this.url} - ${message}`);
|
||||
Log.warn(`${this.logContext}${this.#shortenUrl()} - ${message}`);
|
||||
} else if (status >= 500) {
|
||||
errorType = "SERVER_ERROR";
|
||||
this.serverErrorCount = Math.min(this.serverErrorCount + 1, this.maxRetries);
|
||||
delay = this.reloadInterval * Math.pow(2, this.serverErrorCount);
|
||||
message = `Server error (${status}). Retry #${this.serverErrorCount} in ${Math.round(delay / 60000)} minutes.`;
|
||||
Log.error(`${this.logContext}${this.url} - ${message}`);
|
||||
if (this.serverErrorCount >= this.maxRetries) {
|
||||
delay = this.reloadInterval;
|
||||
message = `Server error (${status}). Max retries reached, retrying at configured interval (${Math.round(delay / 1000)}s).`;
|
||||
} else {
|
||||
delay = HTTPFetcher.calculateBackoffDelay(this.serverErrorCount, {
|
||||
maxDelay: this.reloadInterval
|
||||
});
|
||||
message = `Server error (${status}). Retry #${this.serverErrorCount} in ${Math.round(delay / 1000)}s.`;
|
||||
}
|
||||
Log.error(`${this.logContext}${this.#shortenUrl()} - ${message}`);
|
||||
} else if (status >= 400) {
|
||||
errorType = "CLIENT_ERROR";
|
||||
delay = Math.max(this.reloadInterval * 2, FIFTEEN_MINUTES);
|
||||
message = `Client error (${status}). Retrying in ${Math.round(delay / 60000)} minutes.`;
|
||||
Log.error(`${this.logContext}${this.url} - ${message}`);
|
||||
Log.error(`${this.logContext}${this.#shortenUrl()} - ${message}`);
|
||||
} else {
|
||||
message = `Unexpected HTTP status ${status}.`;
|
||||
Log.error(`${this.logContext}${this.url} - ${message}`);
|
||||
Log.error(`${this.logContext}${this.#shortenUrl()} - ${message}`);
|
||||
}
|
||||
|
||||
return {
|
||||
@@ -293,28 +313,22 @@ class HTTPFetcher extends EventEmitter {
|
||||
const isTimeout = error.name === "AbortError";
|
||||
const message = isTimeout ? `Request timeout after ${this.timeout}ms` : `Network error: ${error.message}`;
|
||||
|
||||
// Apply exponential backoff for network errors
|
||||
this.networkErrorCount = Math.min(this.networkErrorCount + 1, this.maxRetries);
|
||||
const backoffDelay = HTTPFetcher.calculateBackoffDelay(this.networkErrorCount, {
|
||||
maxDelay: this.reloadInterval
|
||||
});
|
||||
nextDelay = backoffDelay;
|
||||
const exhausted = this.networkErrorCount >= this.maxRetries;
|
||||
|
||||
// Truncate URL for cleaner logs
|
||||
let shortUrl = this.url;
|
||||
try {
|
||||
const urlObj = new URL(this.url);
|
||||
shortUrl = `${urlObj.origin}${urlObj.pathname}${urlObj.search.length > 50 ? "?..." : urlObj.search}`;
|
||||
} catch {
|
||||
// If URL parsing fails, use original URL
|
||||
}
|
||||
|
||||
// Gradual log-level escalation: WARN for first 2 attempts, ERROR after
|
||||
const retryMessage = `Retry #${this.networkErrorCount} in ${Math.round(nextDelay / 1000)}s.`;
|
||||
if (this.networkErrorCount <= 2) {
|
||||
Log.warn(`${this.logContext}${shortUrl} - ${message} ${retryMessage}`);
|
||||
if (exhausted) {
|
||||
nextDelay = this.reloadInterval;
|
||||
Log.error(`${this.logContext}${this.#shortenUrl()} - ${message} Max retries reached, retrying at configured interval (${Math.round(nextDelay / 1000)}s).`);
|
||||
} else {
|
||||
Log.error(`${this.logContext}${shortUrl} - ${message} ${retryMessage}`);
|
||||
nextDelay = HTTPFetcher.calculateBackoffDelay(this.networkErrorCount, {
|
||||
maxDelay: this.reloadInterval
|
||||
});
|
||||
const retryMsg = `${this.logContext}${this.#shortenUrl()} - ${message} Retry #${this.networkErrorCount} in ${Math.round(nextDelay / 1000)}s.`;
|
||||
if (this.networkErrorCount <= 2) {
|
||||
Log.warn(retryMsg);
|
||||
} else {
|
||||
Log.error(retryMsg);
|
||||
}
|
||||
}
|
||||
|
||||
const errorInfo = this.#createErrorInfo(
|
||||
@@ -324,18 +338,6 @@ class HTTPFetcher extends EventEmitter {
|
||||
nextDelay,
|
||||
error
|
||||
);
|
||||
|
||||
/**
|
||||
* Error event - fired when fetch fails
|
||||
* @event HTTPFetcher#error
|
||||
* @type {object}
|
||||
* @property {string} message - Error description
|
||||
* @property {number|null} statusCode - HTTP status or null for network errors
|
||||
* @property {number} retryDelay - Ms until next retry
|
||||
* @property {number} retryCount - Number of consecutive server errors
|
||||
* @property {string} url - The URL that was fetched
|
||||
* @property {Error|null} originalError - The original error
|
||||
*/
|
||||
this.emit("error", errorInfo);
|
||||
} finally {
|
||||
clearTimeout(timeoutId);
|
||||
|
||||
@@ -469,3 +469,83 @@ describe("selfSignedCert dispatcher", () => {
|
||||
expect(options.dispatcher).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe("Retry exhaustion fallback", () => {
|
||||
it("should fall back to reloadInterval after network retries exhausted", async () => {
|
||||
server.use(
|
||||
http.get(TEST_URL, () => {
|
||||
return HttpResponse.error();
|
||||
})
|
||||
);
|
||||
|
||||
fetcher = new HTTPFetcher(TEST_URL, { reloadInterval: 300000, maxRetries: 3 });
|
||||
|
||||
const errors = [];
|
||||
fetcher.on("error", (errorInfo) => errors.push(errorInfo));
|
||||
|
||||
// Trigger maxRetries + 1 fetches to reach exhaustion
|
||||
for (let i = 0; i < 4; i++) {
|
||||
await fetcher.fetch();
|
||||
}
|
||||
|
||||
// First retries should use backoff (< reloadInterval)
|
||||
expect(errors[0].retryAfter).toBe(15000);
|
||||
expect(errors[1].retryAfter).toBe(30000);
|
||||
// Third retry hits maxRetries, should fall back to reloadInterval
|
||||
expect(errors[2].retryAfter).toBe(300000);
|
||||
// Subsequent errors stay at reloadInterval
|
||||
expect(errors[3].retryAfter).toBe(300000);
|
||||
});
|
||||
|
||||
it("should fall back to reloadInterval after server error retries exhausted", async () => {
|
||||
server.use(
|
||||
http.get(TEST_URL, () => {
|
||||
return new HttpResponse(null, { status: 503 });
|
||||
})
|
||||
);
|
||||
|
||||
fetcher = new HTTPFetcher(TEST_URL, { reloadInterval: 300000, maxRetries: 3 });
|
||||
|
||||
const errors = [];
|
||||
fetcher.on("error", (errorInfo) => errors.push(errorInfo));
|
||||
|
||||
for (let i = 0; i < 4; i++) {
|
||||
await fetcher.fetch();
|
||||
}
|
||||
|
||||
// First retries should use backoff (< reloadInterval)
|
||||
expect(errors[0].retryAfter).toBe(15000);
|
||||
expect(errors[1].retryAfter).toBe(30000);
|
||||
// Third retry hits maxRetries, should fall back to reloadInterval
|
||||
expect(errors[2].retryAfter).toBe(300000);
|
||||
// Subsequent errors stay at reloadInterval
|
||||
expect(errors[3].retryAfter).toBe(300000);
|
||||
});
|
||||
|
||||
it("should reset network error count on success", async () => {
|
||||
let requestCount = 0;
|
||||
server.use(
|
||||
http.get(TEST_URL, () => {
|
||||
requestCount++;
|
||||
if (requestCount <= 2) return HttpResponse.error();
|
||||
return HttpResponse.text("ok");
|
||||
})
|
||||
);
|
||||
|
||||
fetcher = new HTTPFetcher(TEST_URL, { reloadInterval: 300000, maxRetries: 3 });
|
||||
|
||||
const errors = [];
|
||||
fetcher.on("error", (errorInfo) => errors.push(errorInfo));
|
||||
|
||||
// Two failures with backoff
|
||||
await fetcher.fetch();
|
||||
await fetcher.fetch();
|
||||
expect(errors).toHaveLength(2);
|
||||
expect(errors[0].retryAfter).toBe(15000);
|
||||
expect(errors[1].retryAfter).toBe(30000);
|
||||
|
||||
// Success resets counter
|
||||
await fetcher.fetch();
|
||||
expect(fetcher.networkErrorCount).toBe(0);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user