From 9096d1dfd3a0adaebf43671a380d1fd2227fb545 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Thu, 17 Sep 2026 08:50:51 -0400 Subject: [PATCH] fix(web): do not retry plugin requests that got an HTTP answer PluginAPI.request wrapped everything that was not a structured error as NETWORK_ERROR: a proxy's 502 HTML page (response.json() throws) and a JSON error without error_code included. Check & Update All retries NETWORK_ERROR, so those updates were re-sent five more times with backoff, contrary to the #587 contract that an HTTP error response is the server's answer. NETWORK_ERROR now means only that fetch() rejected. Any HTTP response without an error_code, or with a body that is not JSON, is API_ERROR with the HTTP status attached. Tested against the shipped api_client.js. Co-Authored-By: Claude Opus 5 --- test/js/README.md | 2 +- test/js/unit/test_update_all.js | 40 +++++++++++++ .../static/v3/js/plugins/api_client.js | 59 +++++++++++++------ 3 files changed, 82 insertions(+), 19 deletions(-) diff --git a/test/js/README.md b/test/js/README.md index 9d7c3b95..51a12879 100644 --- a/test/js/README.md +++ b/test/js/README.md @@ -35,7 +35,7 @@ nothing is listening, so it stays useful in a bare checkout. | Suite | Needs a server | Covers | |---|---|---| | `unit/test_list_filter.js` | no | `ListFilter` search/filter/sort/count/sticky, and the installed-plugins config **extracted verbatim** from `plugins_manager.js` so the test can't drift from it | -| `unit/test_update_all.js` | no | `PluginInstallManager.updateAll` from `plugins/install_manager.js`: Check & Update All sends only plugin ids (never `starlark:` app entries), and re-sends a request that got no HTTP answer (web service restarting) instead of skipping that plugin. Also run by `test/web_interface/test_update_all_plugins.py` so CI covers it | +| `unit/test_update_all.js` | no | `PluginInstallManager.updateAll` from `plugins/install_manager.js`: Check & Update All sends only plugin ids (never `starlark:` app entries), re-sends a request that got no HTTP answer (web service restarting) instead of skipping that plugin, never re-sends one that got any HTTP answer (the real `api_client.js` classifies a proxy 502 or a JSON error without `error_code` as `API_ERROR`), and counts a no-op update as already up to date in the summary. Also run by `test/web_interface/test_update_all_plugins.py` so CI covers it | | `unit/test_render_cards.js` | no | `renderInstalledCards` markup, both empty states, and HTML-escaping of hostile plugin metadata | | `unit/test_style_editor_element_keys.js` | no | `elementKeys()`/`styleRows()`/`positionRows()` from `widgets/style-editor.js`: every `customization.layout` entry gets exactly one row -- paired with its style element through core's `x-layout-key` (so `score` belongs to `score_text`, not a second row), or a position row of its own, leaves included -- since the widget claims the whole `layout` block from the generic fallback renderer | | `unit/test_style_editor_layout_leaf_columns.js` | no | `columnsFor()` from `widgets/style-editor.js`: a layout-only key whose own value is a leaf (no x/y sub-object, e.g. a `show_logo` toggle) gets a self-keyed column instead of a blank, uneditable row | diff --git a/test/js/unit/test_update_all.js b/test/js/unit/test_update_all.js index 539fefc3..f9c2cbbd 100644 --- a/test/js/unit/test_update_all.js +++ b/test/js/unit/test_update_all.js @@ -146,6 +146,46 @@ const noSleep = { sleep: async () => {} }; ok('...and is reported as a failure', results.find(x => x.pluginId === 'static-image').success === false); } + console.log('\nthe real api_client.js: only a missing HTTP answer is retried'); + { + // The fake PluginAPI above decides error_code itself. This runs the shipped + // client so its classification is what gets tested: a proxy's 502 page or + // a JSON 500 without error_code used to come back as NETWORK_ERROR and be + // re-sent five more times. + const PluginAPI = require(path.join(V3, 'js/plugins/api_client.js')); + const run = async (makeFetch) => { + let requests = 0, sleeps = 0; + global.fetch = async () => { requests++; return makeFetch(requests); }; + global.window = { PluginAPI, installedPlugins: [{ id: 'stock-news' }] }; + const results = await Manager.updateAll(null, { sleep: async () => { sleeps++; }, retryDelaysMs: [1, 1, 1] }); + return { requests, sleeps, result: results[0] }; + }; + const httpAnswer = (status, json) => ({ ok: status < 400, status, json }); + + let r = await run(() => httpAnswer(502, async () => { throw new SyntaxError('Unexpected token <'); })); + ok('a 502 with an HTML body is sent once', r.requests === 1 && r.sleeps === 0, r); + ok('...and is an API_ERROR carrying the status, not NETWORK_ERROR', + r.result.success === false && r.result.error.error_code === 'API_ERROR' && r.result.error.status === 502, r.result.error); + + r = await run(() => httpAnswer(500, async () => ({ status: 'error', message: 'boom' }))); + ok('a JSON 500 without error_code is sent once', r.requests === 1 && r.sleeps === 0, r); + ok('...and keeps the server message', r.result.error.message === 'boom', r.result.error); + + r = await run(() => httpAnswer(500, async () => ({ status: 'error', error_code: 'PLUGIN_UPDATE_FAILED', message: 'x' }))); + ok('a structured error is passed through unchanged', + r.requests === 1 && r.result.error.error_code === 'PLUGIN_UPDATE_FAILED', r.result.error); + + r = await run((n) => { + if (n === 1) throw new TypeError('Failed to fetch'); + return httpAnswer(200, async () => ({ status: 'success', message: 'ok', data: { update_status: 'updated' } })); + }); + ok('a fetch() that rejects is NETWORK_ERROR and re-sent', r.requests === 2 && r.sleeps === 1 && r.result.success, r); + + r = await run(() => httpAnswer(200, async () => { throw new SyntaxError('Unexpected end of JSON input'); })); + ok('an unreadable 200 is not retried either', r.requests === 1 && r.result.error.error_code === 'API_ERROR', r); + delete global.fetch; + } + console.log('\nsummary: a no-op update is not counted as updated'); { const answer = (update_status, message) => ({ success: true, result: { status: 'success', message, data: { update_status } } }); diff --git a/web_interface/static/v3/js/plugins/api_client.js b/web_interface/static/v3/js/plugins/api_client.js index fd33ce12..0e54f03b 100644 --- a/web_interface/static/v3/js/plugins/api_client.js +++ b/web_interface/static/v3/js/plugins/api_client.js @@ -153,31 +153,54 @@ const PluginAPI = { options.body = JSON.stringify(data); } + // NETWORK_ERROR means only that fetch() itself rejected: no HTTP + // answer arrived (connection refused/reset, e.g. the web service + // restarting). Callers retry that (install_manager.js updateAll). + // Any HTTP response is the server's -- or a proxy's -- answer, so + // a 502 HTML page or a JSON error without error_code is API_ERROR + // and is not retried. + let response; try { - const response = await fetch(url, options); - const responseData = await response.json(); - - if (!response.ok) { - // Handle structured errors - if (responseData.error_code) { - throw responseData; - } - throw new Error(responseData.message || `HTTP ${response.status}`); - } - - return responseData; + response = await fetch(url, options); } catch (error) { - // Re-throw structured errors - if (error.error_code) { - throw error; - } - // Wrap network errors throw { error_code: 'NETWORK_ERROR', - message: error.message || 'Network error', + message: (error && error.message) || 'Network error', original_error: error }; } + + let responseData = null; + let parseError = null; + try { + responseData = await response.json(); + } catch (error) { + parseError = error; + } + + if (!response.ok) { + // Handle structured errors + if (responseData && responseData.error_code) { + throw responseData; + } + throw { + error_code: 'API_ERROR', + status: response.status, + message: (responseData && responseData.message) || `HTTP ${response.status}`, + original_error: parseError || undefined + }; + } + + if (parseError) { + throw { + error_code: 'API_ERROR', + status: response.status, + message: `Unreadable response from the server (HTTP ${response.status})`, + original_error: parseError + }; + } + + return responseData; }; // Use throttling for GET requests, immediate execution for POST/PUT/DELETE