From d3c248304e01fb33c0d4dcc9b359dadc67c43d12 Mon Sep 17 00:00:00 2001 From: Dennis Jakobsen Date: Mon, 14 Sep 2026 16:12:23 +0200 Subject: [PATCH 1/4] fix(app.js): stop api()'s discarded finally-cleanup promise from double-rejecting api() does `promise.finally(() => _inflight.delete(path))` and discards the derived promise it returns. `.finally()` mirrors the outcome of the promise it's attached to, so when a request rejects, that derived promise rejects too, with nothing ever attached to observe it -- a second, orphaned rejection alongside the one every real caller already handles via the returned `promise`. Browsers only surface this as a benign `Uncaught (in promise)` console warning; it was caught here because Node's stricter default (an unhandled rejection crashes the process) has no browser equivalent, and surfaced while building a behavioral test for #1375's scope-stats fetch caching. Fix: attach a no-op `.catch(() => {})` to the discarded `.finally()` promise. This only consumes that derived promise's mirrored rejection -- the `promise` returned to callers is a different object and its resolution/rejection to them is completely unaffected. Reproduced fail-before / pass-after with `node --unhandled-rejections =strict` against a minimal harness loading the real api(). New test covers success, failure semantics, the no-extra-rejection fix itself (via a child-process check under the same strict flag, with a negative control proving that same check does catch a genuine unhandled rejection), in-flight cleanup after both outcomes, retry-after-failure, concurrent-request dedup, and TTL cache behavior -- all against the real, unmodified api()/_apiCache/_inflight. Co-Authored-By: Claude Sonnet 5 --- public/app.js | 8 +- test-app-api-inflight-cleanup-rejection.js | 231 +++++++++++++++++++++ 2 files changed, 238 insertions(+), 1 deletion(-) create mode 100644 test-app-api-inflight-cleanup-rejection.js diff --git a/public/app.js b/public/app.js index 52bab2ce8..46ebd261c 100644 --- a/public/app.js +++ b/public/app.js @@ -200,7 +200,13 @@ async function api(path, { ttl = 0, bust = false } = {}) { } })(); _inflight.set(path, promise); - promise.finally(() => _inflight.delete(path)); + // `.finally()` returns its own derived promise that mirrors `promise`'s + // outcome; discarding it uncaught leaves the real caller's rejection + // (delivered via the returned `promise` below, unaffected by this) + // duplicated as a second, unobserved rejection on this derived one. + // The `.catch()` here only silences that duplicate -- it does not + // touch `promise` itself or its resolution to callers. + promise.finally(() => _inflight.delete(path)).catch(() => {}); return promise; } diff --git a/test-app-api-inflight-cleanup-rejection.js b/test-app-api-inflight-cleanup-rejection.js new file mode 100644 index 000000000..7a14dcdd9 --- /dev/null +++ b/test-app-api-inflight-cleanup-rejection.js @@ -0,0 +1,231 @@ +/** + * app.js's api() helper: the discarded `.finally()` cleanup promise must + * not produce a second, unobserved rejection alongside the one the real + * caller already handles. + * + * api() does: + * _inflight.set(path, promise); + * promise.finally(() => _inflight.delete(path)); + * return promise; + * + * `.finally()` returns its own derived promise mirroring `promise`'s + * outcome. That derived promise was discarded uncaught, so when a + * request rejects, BOTH the returned `promise` (correctly awaited/caught + * by every real caller) AND the orphaned `.finally()` promise reject -- + * the second one with nothing ever attached to observe it. In a browser + * this only produces a benign `Uncaught (in promise)` console warning + * (nothing reads that derived promise's value), but it is still a real, + * avoidable defect: Node's stricter default (an unhandled rejection + * crashes the process) has no browser equivalent and is what surfaced + * this while testing #1375's scope-stats fetch behavior. + * + * Fix: `promise.finally(() => _inflight.delete(path)).catch(() => {})`. + * The added `.catch()` only consumes the *derived* promise's mirrored + * rejection -- it is a different promise object from the `promise` + * returned to callers, so callers' error handling is completely + * unaffected (proven by scenario B below matching the pre-fix error + * text exactly). + * + * Scenarios A-H below load the REAL, unmodified public/app.js via vm + * (only `fetch` is stubbed) so the real api()/_apiCache/_inflight logic + * runs unmodified from this test's perspective. + */ +'use strict'; + +const vm = require('vm'); +const fs = require('fs'); +const path = require('path'); +const assert = require('assert'); +const { spawnSync } = require('child_process'); + +let passed = 0, failed = 0; +function check(cond, msg) { + if (cond) { passed++; console.log(' ✓ ' + msg); } + else { failed++; console.error(' ✗ ' + msg); } +} +async function checkAsync(name, fn) { + try { + await fn(); + passed++; + console.log(' ✓ ' + name); + } catch (e) { + failed++; + console.error(' ✗ ' + name + ': ' + e.message); + } +} + +const APP_JS_PATH = path.join(__dirname, 'public', 'app.js'); + +function makeSandbox() { + const fetchLog = []; + let fetchBehavior = () => ({ ok: true, status: 200, json: async () => ({}), headers: { get: () => null } }); + + const ctx = { + window: { addEventListener: () => {}, dispatchEvent: () => {} }, + document: { + readyState: 'complete', + getElementById: () => null, + addEventListener: () => {}, + querySelectorAll: () => [], + querySelector: () => null, + createElement: () => ({ style: {} }), + head: { appendChild: () => {} }, + }, + console, Date, Promise, Map, Set, JSON, Math, Error, TypeError, Array, Object, String, Number, RegExp, + parseInt, parseFloat, isNaN, isFinite, encodeURIComponent, decodeURIComponent, + setTimeout: (fn) => setTimeout(fn, 0), clearTimeout: () => {}, + setInterval: () => 1, clearInterval: () => {}, + performance: { now: () => Date.now() }, + location: { hash: '' }, + addEventListener: () => {}, + dispatchEvent: () => {}, + }; + ctx.fetch = function (url) { + fetchLog.push(url); + const r = fetchBehavior(url); + return r instanceof Promise ? r : Promise.resolve(r); + }; + vm.createContext(ctx); + vm.runInContext(fs.readFileSync(APP_JS_PATH, 'utf8'), ctx); + for (const k of Object.keys(ctx.window)) ctx[k] = ctx.window[k]; + + return { + api: ctx.api, + invalidateApiCache: ctx.invalidateApiCache, + // app.js's own top-level `fetch('/api/config/cache')` fires on load; + // scoping every count to a scenario-private path keeps that separate. + countFor: (p) => fetchLog.filter((u) => u === '/api' + p).length, + setFetchBehavior: (fn) => { fetchBehavior = fn; }, + }; +} + +// Deterministic drain of already-queued microtasks/macrotasks -- no +// wall-clock sleeps. +function flush(times) { + let p = Promise.resolve(); + for (let i = 0; i < (times || 10); i++) p = p.then(() => new Promise((r) => setImmediate(r))); + return p; +} + +(async () => { + console.log('\n=== api() orphaned .finally() cleanup-rejection fix ==='); + + await checkAsync('A. Success: api() resolves with the real fetched data, unchanged', async () => { + const h = makeSandbox(); + h.setFetchBehavior(() => ({ ok: true, status: 200, json: async () => ({ hello: 'world' }), headers: { get: () => null } })); + const data = await h.api('/test/a-success'); + assert.deepStrictEqual(data, { hello: 'world' }); + }); + + await checkAsync('B. Failure: the caller still gets the exact same rejection semantics as before the fix', async () => { + const h = makeSandbox(); + h.setFetchBehavior(() => ({ ok: false, status: 500, json: async () => ({}), headers: { get: () => null } })); + await assert.rejects(() => h.api('/test/b-fail'), /^Error: API 500: \/test\/b-fail$/); + }); + + await checkAsync('C. A handled request failure produces NO additional unhandled rejection (child process, --unhandled-rejections=strict)', async () => { + const script = ` + const vm = require('vm'); + const fs = require('fs'); + const ctx = { + window: { addEventListener: () => {}, dispatchEvent: () => {} }, + document: { readyState: 'complete', getElementById: () => null, addEventListener: () => {}, querySelectorAll: () => [] }, + console, Date, Promise, Map, Set, JSON, Math, Error, TypeError, + parseInt, isFinite, encodeURIComponent, decodeURIComponent, + setTimeout: (fn) => setTimeout(fn, 0), clearTimeout: () => {}, + performance: { now: () => Date.now() }, + location: { hash: '' }, addEventListener: () => {}, dispatchEvent: () => {}, + }; + ctx.fetch = () => Promise.resolve({ ok: false, status: 500, headers: { get: () => null }, json: async () => ({}) }); + vm.createContext(ctx); + vm.runInContext(fs.readFileSync(${JSON.stringify(APP_JS_PATH)}, 'utf8'), ctx); + for (const k of Object.keys(ctx.window)) ctx[k] = ctx.window[k]; + (async () => { + try { await ctx.api('/c-handled-fail'); process.exitCode = 1; } + catch (e) { /* caller correctly handles it -- this is the ONLY place the rejection should be observed */ } + let p = Promise.resolve(); + for (let i = 0; i < 10; i++) p = p.then(() => new Promise((r) => setImmediate(r))); + await p; + })(); + `; + const r = spawnSync(process.execPath, ['--unhandled-rejections=strict', '-e', script], { timeout: 10000, encoding: 'utf8' }); + assert.strictEqual(r.status, 0, + 'expected the child process to exit 0 (no unhandled rejection) under --unhandled-rejections=strict; ' + + 'got status=' + r.status + ' stderr=' + r.stderr); + }); + + await checkAsync('D. In-flight entry is cleared after a SUCCESSFUL request (no stale dedup blocking a later independent call)', async () => { + const h = makeSandbox(); + h.setFetchBehavior(() => ({ ok: true, status: 200, json: async () => ({ n: 1 }), headers: { get: () => null } })); + await h.api('/test/d-success'); // ttl:0 (default) -> no cache write either + await h.api('/test/d-success'); + assert.strictEqual(h.countFor('/test/d-success'), 2, + 'expected 2 independent fetches (in-flight entry must not still be present from the first, already-settled call)'); + }); + + await checkAsync('E. In-flight entry is cleared after a FAILED request, and the next attempt can legitimately succeed', async () => { + const h = makeSandbox(); + let attempts = 0; + h.setFetchBehavior(() => { + attempts++; + return attempts === 1 + ? { ok: false, status: 500, json: async () => ({}), headers: { get: () => null } } + : { ok: true, status: 200, json: async () => ({ recovered: true }), headers: { get: () => null } }; + }); + await assert.rejects(() => h.api('/test/e-retry')); + const data = await h.api('/test/e-retry'); // must be a fresh fetch, not the stale rejected in-flight promise + assert.deepStrictEqual(data, { recovered: true }); + assert.strictEqual(h.countFor('/test/e-retry'), 2); + }); + + await checkAsync('F. Concurrent requests for the same path are still deduplicated via _inflight', async () => { + const h = makeSandbox(); + let resolveFetch; + h.setFetchBehavior(() => new Promise((res) => { + resolveFetch = () => res({ ok: true, status: 200, json: async () => ({ shared: true }), headers: { get: () => null } }); + })); + const p1 = h.api('/test/f-dedup'); + const p2 = h.api('/test/f-dedup'); // fired before p1 settles -> must reuse the same in-flight promise + await flush(3); + resolveFetch(); + const [d1, d2] = await Promise.all([p1, p2]); + assert.deepStrictEqual(d1, { shared: true }); + assert.deepStrictEqual(d2, { shared: true }); + assert.strictEqual(h.countFor('/test/f-dedup'), 1, 'expected exactly 1 real fetch for 2 concurrent identical calls'); + }); + + await checkAsync('G. TTL cache still serves a hit within TTL, and still refetches after invalidation', async () => { + const h = makeSandbox(); + h.setFetchBehavior(() => ({ ok: true, status: 200, json: async () => ({ cached: true }), headers: { get: () => null } })); + await h.api('/test/g-ttl', { ttl: 30000 }); + await h.api('/test/g-ttl', { ttl: 30000 }); + assert.strictEqual(h.countFor('/test/g-ttl'), 1, 'second call within TTL must be served from cache, not refetched'); + h.invalidateApiCache('/test/g-ttl'); + await h.api('/test/g-ttl', { ttl: 30000 }); + assert.strictEqual(h.countFor('/test/g-ttl'), 2, 'after invalidation, the next call must be a real fetch'); + }); + + await checkAsync('H. The harness itself does not hide a real unhandled rejection (no suppressing global handler; the same strict-mode check DOES catch a genuine one)', async () => { + assert.strictEqual(process.listenerCount('unhandledRejection'), 0, + 'this test file must not install any process-wide unhandledRejection handler'); + // Same technique as C, but with a deliberately uncaught rejection + // unrelated to api() -- proves the check is discriminating, not + // vacuously green regardless of what runs inside it. + const script = ` + Promise.reject(new Error('deliberate-uncaught-control')); + let p = Promise.resolve(); + for (let i = 0; i < 10; i++) p = p.then(() => new Promise((r) => setImmediate(r))); + p.then(() => { process.exitCode = 0; }); + `; + const r = spawnSync(process.execPath, ['--unhandled-rejections=strict', '-e', script], { timeout: 10000, encoding: 'utf8' }); + assert.notStrictEqual(r.status, 0, + 'expected a deliberately uncaught rejection to make the child process exit non-zero under --unhandled-rejections=strict -- ' + + 'got status=' + r.status + ' (if this is 0, the harness technique used in scenario C cannot be trusted)'); + }); + + console.log('\n=== Summary ==='); + console.log(' Passed: ' + passed); + console.log(' Failed: ' + failed); + console.log('\napi()-inflight-cleanup-rejection ' + (failed === 0 ? 'PASS' : 'FAIL')); + process.exitCode = failed === 0 ? 0 : 1; +})(); From 86645756f27754392daba4cfad6fed3cee100f49 Mon Sep 17 00:00:00 2001 From: Dennis Jakobsen Date: Mon, 14 Sep 2026 16:28:05 +0200 Subject: [PATCH 2/4] test: fix two false-positive gaps found in independent review Round-2 (Opus) review of d3c24830 blocked test registration on two harness bugs, both confirmed by reproduction before this fix and again after it: 1. Broken in-flight dedup (removing `if (_inflight.has(path)) return _inflight.get(path);`) left scenario F's second Promise.all() branch permanently pending. Nothing else kept the event loop alive, so Node drained and exited 0 without ever reaching G/H or printing a summary -- a silent false pass. Fixed two ways: process.exitCode is now set to 1 immediately and only flipped to 0 after explicitly confirming all 8 named scenarios (A-H) ran to completion and passed; scenario F also now asserts exactly 1 fetch happened before resolving, catching this mutation instantly instead of relying on the fallback. A non-unref'd watchdog timer additionally guards against a genuine hang (a real pending macrotask keeps the event loop alive, so Node cannot silently exit early while it's armed) and is cleared on the normal completion path. 2. Scenario C's child script accepted ANY thrown error via a bare catch, so pointing it at a nonexistent `ctx.apiTypo` (or making api() throw before ever calling fetch) still printed a pass. The child now explicitly verifies api is a function, checks the exact rejection message, checks the exact fetch count and URL, and only then writes a unique success marker; the parent requires that exact marker plus a clean exit, no signal, and no stderr. Scenario H was narrowed to what it actually proves (the strict-mode child-process technique detects a deliberate unhandled rejection) and now checks for that rejection's specific message in stderr, so an unrelated child crash can no longer count as a passing negative control; its dead `exitCode = 0` line is removed. Re-verified: 12/12 clean runs (~0.1s each, full A-H every time). Both original false positives now fail correctly. Two new mutations checked too: api() failing before its fetch call, and removing the production fix's `.catch()` on the discarded `.finally()` promise -- the latter's failure output names the exact same bug (`API 500: ...` at the orphaned-rejection site), not a generic crash. No leftover processes after any run. public/app.js is untouched (byte-identical to d3c24830). Co-Authored-By: Claude Sonnet 5 --- test-app-api-inflight-cleanup-rejection.js | 213 ++++++++++++++++----- 1 file changed, 170 insertions(+), 43 deletions(-) diff --git a/test-app-api-inflight-cleanup-rejection.js b/test-app-api-inflight-cleanup-rejection.js index 7a14dcdd9..7883eb081 100644 --- a/test-app-api-inflight-cleanup-rejection.js +++ b/test-app-api-inflight-cleanup-rejection.js @@ -21,29 +21,65 @@ * * Fix: `promise.finally(() => _inflight.delete(path)).catch(() => {})`. * The added `.catch()` only consumes the *derived* promise's mirrored - * rejection -- it is a different promise object from the `promise` - * returned to callers, so callers' error handling is completely - * unaffected (proven by scenario B below matching the pre-fix error - * text exactly). + * rejection -- it is a different promise object from the promise chain + * returned to callers (api() is `async function`, so every caller + * actually holds a promise that FOLLOWS `promise`, never `promise` + * itself -- but that followed promise mirrors the same resolution, so + * callers' error handling is unaffected either way, proven by scenario + * B below matching the pre-fix error text exactly). * * Scenarios A-H below load the REAL, unmodified public/app.js via vm * (only `fetch` is stubbed) so the real api()/_apiCache/_inflight logic * runs unmodified from this test's perspective. + * + * --- Completion guarantee (round 2 review fix) --- + * A prior version of this file set `process.exitCode` only at the very + * end. If an in-flight dedup regression left one branch of a + * Promise.all() permanently pending (scenario F), nothing else kept the + * event loop alive, so Node drained and exited 0 WITHOUT ever reaching + * F/G/H or the summary -- a silent false pass. Two independent guards + * now prevent that: + * 1. `process.exitCode = 1` is set immediately, before anything else + * runs, and only flipped to 0 after every named scenario has been + * confirmed to have run AND all of them passed. + * 2. A watchdog `setTimeout` (not unref'd) is armed for the whole + * run's duration. A real, non-unref'd timer is a pending macrotask, + * so it keeps the event loop alive even if some other promise + * chain stalls -- Node cannot silently drain and exit while it is + * pending. If the suite hasn't finished by the deadline, the + * watchdog itself fails loudly and exits 1. It is cleared on the + * normal completion path, so a healthy run's timing is unaffected. */ 'use strict'; +process.exitCode = 1; // Flipped to 0 only after every scenario is confirmed complete AND passing. + const vm = require('vm'); const fs = require('fs'); const path = require('path'); const assert = require('assert'); const { spawnSync } = require('child_process'); +const WATCHDOG_MS = 15000; +const watchdog = setTimeout(() => { + console.error( + '\n✗ WATCHDOG: the suite did not finish within ' + WATCHDOG_MS + 'ms. ' + + 'A real, non-unref\'d timer (this one) is a pending macrotask, so this ' + + 'firing means the event loop was otherwise still alive -- something is ' + + 'genuinely hung (not the historical "silent early exit 0" failure mode, ' + + 'which this timer separately prevents just by existing). Failing loudly ' + + 'instead of hanging CI indefinitely.' + ); + process.exitCode = 1; + process.exit(1); +}, WATCHDOG_MS); + +const EXPECTED_SCENARIOS = ['A', 'B', 'C', 'D', 'E', 'F', 'G', 'H']; +const ranScenarios = new Set(); + let passed = 0, failed = 0; -function check(cond, msg) { - if (cond) { passed++; console.log(' ✓ ' + msg); } - else { failed++; console.error(' ✗ ' + msg); } -} async function checkAsync(name, fn) { + const label = (/^([A-H])\./.exec(name) || [])[1]; try { await fn(); passed++; @@ -51,6 +87,8 @@ async function checkAsync(name, fn) { } catch (e) { failed++; console.error(' ✗ ' + name + ': ' + e.message); + } finally { + if (label) ranScenarios.add(label); } } @@ -107,6 +145,81 @@ function flush(times) { return p; } +// Shared child-process source for scenario C. Deliberately verbose and +// self-checking rather than a bare try/catch: a wrong/missing api(), an +// error thrown before fetch, or a mismatched failure must each produce +// their OWN distinguishable failure line, and only the fully-verified +// exact path may print SUCCESS_MARKER. An empty catch cannot let any of +// this slide, because there is no catch-and-ignore left in this script; +// every branch either explicitly fails or explicitly proceeds. +const C_SUCCESS_MARKER = 'REACHED_EXPECTED_SUCCESS_MARKER_9f3a1c'; +function buildScenarioCChildScript(appJsPath) { + return ` + const vm = require('vm'); + const fs = require('fs'); + const fetchLog = []; + const ctx = { + window: { addEventListener: () => {}, dispatchEvent: () => {} }, + document: { readyState: 'complete', getElementById: () => null, addEventListener: () => {}, querySelectorAll: () => [] }, + console, Date, Promise, Map, Set, JSON, Math, Error, TypeError, + parseInt, isFinite, encodeURIComponent, decodeURIComponent, + setTimeout: (fn) => setTimeout(fn, 0), clearTimeout: () => {}, + performance: { now: () => Date.now() }, + location: { hash: '' }, addEventListener: () => {}, dispatchEvent: () => {}, + }; + ctx.fetch = function (url) { + fetchLog.push(url); + return Promise.resolve({ ok: false, status: 500, headers: { get: () => null }, json: async () => ({}) }); + }; + vm.createContext(ctx); + vm.runInContext(fs.readFileSync(${JSON.stringify(appJsPath)}, 'utf8'), ctx); + for (const k of Object.keys(ctx.window)) ctx[k] = ctx.window[k]; + + (async () => { + if (typeof ctx.api !== 'function') { + console.error('CHILD_FAIL: ctx.api is not a function (got ' + typeof ctx.api + ') -- app.js did not expose the real api() helper'); + process.exitCode = 1; + return; + } + + const PATH = '/c-handled-fail'; + const EXPECTED_MESSAGE = 'API 500: ' + PATH; + let rejection = null; + try { + const result = await ctx.api(PATH); + console.error('CHILD_FAIL: expected api(' + PATH + ') to reject, but it resolved with ' + JSON.stringify(result)); + process.exitCode = 1; + return; + } catch (e) { + rejection = e; + } + + if (!rejection || rejection.message !== EXPECTED_MESSAGE) { + console.error('CHILD_FAIL: expected rejection message ' + JSON.stringify(EXPECTED_MESSAGE) + ', got ' + JSON.stringify(rejection && rejection.message)); + process.exitCode = 1; + return; + } + + const matchingFetches = fetchLog.filter((u) => u === '/api' + PATH); + if (matchingFetches.length !== 1) { + console.error('CHILD_FAIL: expected exactly 1 fetch to /api' + PATH + ', got ' + matchingFetches.length + ': ' + JSON.stringify(fetchLog)); + process.exitCode = 1; + return; + } + + // Drain queued microtasks/macrotasks so the orphaned .finally() + // promise's rejection (if the production fix is absent) gets a + // chance to surface as an unhandled rejection BEFORE we declare + // success -- this is the actual condition under test. + let p = Promise.resolve(); + for (let i = 0; i < 10; i++) p = p.then(() => new Promise((r) => setImmediate(r))); + await p; + + console.log('${C_SUCCESS_MARKER}'); + })(); + `; +} + (async () => { console.log('\n=== api() orphaned .finally() cleanup-rejection fix ==='); @@ -124,34 +237,20 @@ function flush(times) { }); await checkAsync('C. A handled request failure produces NO additional unhandled rejection (child process, --unhandled-rejections=strict)', async () => { - const script = ` - const vm = require('vm'); - const fs = require('fs'); - const ctx = { - window: { addEventListener: () => {}, dispatchEvent: () => {} }, - document: { readyState: 'complete', getElementById: () => null, addEventListener: () => {}, querySelectorAll: () => [] }, - console, Date, Promise, Map, Set, JSON, Math, Error, TypeError, - parseInt, isFinite, encodeURIComponent, decodeURIComponent, - setTimeout: (fn) => setTimeout(fn, 0), clearTimeout: () => {}, - performance: { now: () => Date.now() }, - location: { hash: '' }, addEventListener: () => {}, dispatchEvent: () => {}, - }; - ctx.fetch = () => Promise.resolve({ ok: false, status: 500, headers: { get: () => null }, json: async () => ({}) }); - vm.createContext(ctx); - vm.runInContext(fs.readFileSync(${JSON.stringify(APP_JS_PATH)}, 'utf8'), ctx); - for (const k of Object.keys(ctx.window)) ctx[k] = ctx.window[k]; - (async () => { - try { await ctx.api('/c-handled-fail'); process.exitCode = 1; } - catch (e) { /* caller correctly handles it -- this is the ONLY place the rejection should be observed */ } - let p = Promise.resolve(); - for (let i = 0; i < 10; i++) p = p.then(() => new Promise((r) => setImmediate(r))); - await p; - })(); - `; + const script = buildScenarioCChildScript(APP_JS_PATH); const r = spawnSync(process.execPath, ['--unhandled-rejections=strict', '-e', script], { timeout: 10000, encoding: 'utf8' }); + assert.strictEqual(r.error, undefined, + 'child process failed to spawn: ' + (r.error && r.error.message)); + assert.strictEqual(r.signal, null, + 'child process was killed by a signal (likely the 10s timeout) instead of exiting normally: ' + r.signal); assert.strictEqual(r.status, 0, - 'expected the child process to exit 0 (no unhandled rejection) under --unhandled-rejections=strict; ' + - 'got status=' + r.status + ' stderr=' + r.stderr); + 'expected the child to exit 0 (no unhandled rejection) under --unhandled-rejections=strict; ' + + 'got status=' + r.status + ' stdout=' + r.stdout + ' stderr=' + r.stderr); + assert.ok((r.stdout || '').includes(C_SUCCESS_MARKER), + 'child exited 0 but never printed the success marker -- it must have returned early without ' + + 'actually exercising and verifying the real api() call. stdout=' + r.stdout + ' stderr=' + r.stderr); + assert.strictEqual((r.stderr || '').trim(), '', + 'expected no stderr output on the success path; got: ' + r.stderr); }); await checkAsync('D. In-flight entry is cleared after a SUCCESSFUL request (no stale dedup blocking a later independent call)', async () => { @@ -187,6 +286,13 @@ function flush(times) { const p1 = h.api('/test/f-dedup'); const p2 = h.api('/test/f-dedup'); // fired before p1 settles -> must reuse the same in-flight promise await flush(3); + // If dedup were broken, p2 would have triggered its OWN fetch here, + // silently reassigning `resolveFetch` to the second call's resolver + // and leaving p1 permanently pending. Fail loudly on that instead of + // calling a possibly-stale resolver and hanging inside Promise.all. + assert.strictEqual(h.countFor('/test/f-dedup'), 1, + 'expected exactly 1 real fetch to have been made BEFORE resolving (dedup must reuse the in-flight promise, ' + + 'not start a second real fetch that would leave the first caller\'s promise permanently pending)'); resolveFetch(); const [d1, d2] = await Promise.all([p1, p2]); assert.deepStrictEqual(d1, { shared: true }); @@ -205,27 +311,48 @@ function flush(times) { assert.strictEqual(h.countFor('/test/g-ttl'), 2, 'after invalidation, the next call must be a real fetch'); }); - await checkAsync('H. The harness itself does not hide a real unhandled rejection (no suppressing global handler; the same strict-mode check DOES catch a genuine one)', async () => { + await checkAsync('H. The strict-mode child-process technique used in C genuinely detects a deliberate unhandled rejection (and this file installs no suppressing handler)', async () => { assert.strictEqual(process.listenerCount('unhandledRejection'), 0, 'this test file must not install any process-wide unhandledRejection handler'); - // Same technique as C, but with a deliberately uncaught rejection - // unrelated to api() -- proves the check is discriminating, not - // vacuously green regardless of what runs inside it. + // Deliberately uncaught rejection, unrelated to api(), with a unique + // message so the parent can confirm the crash was caused BY THIS + // rejection specifically -- not by some unrelated child-process + // failure (a syntax error, a missing module, etc.) that would also + // produce a nonzero exit but prove nothing about the technique. + const marker = 'deliberate-uncaught-control-7d2e'; const script = ` - Promise.reject(new Error('deliberate-uncaught-control')); + Promise.reject(new Error('${marker}')); let p = Promise.resolve(); for (let i = 0; i < 10; i++) p = p.then(() => new Promise((r) => setImmediate(r))); - p.then(() => { process.exitCode = 0; }); + p.then(() => { console.log('SHOULD_NOT_REACH_HERE_IF_STRICT_MODE_WORKS'); }); `; const r = spawnSync(process.execPath, ['--unhandled-rejections=strict', '-e', script], { timeout: 10000, encoding: 'utf8' }); + assert.strictEqual(r.error, undefined, + 'child process failed to spawn: ' + (r.error && r.error.message)); assert.notStrictEqual(r.status, 0, - 'expected a deliberately uncaught rejection to make the child process exit non-zero under --unhandled-rejections=strict -- ' + + 'expected the deliberate unhandled rejection to make the child exit non-zero under --unhandled-rejections=strict -- ' + 'got status=' + r.status + ' (if this is 0, the harness technique used in scenario C cannot be trusted)'); + assert.ok((r.stderr || '').includes(marker), + 'expected the crash to be caused SPECIFICALLY by our deliberate rejection (stderr should mention "' + marker + '"), ' + + 'not an unrelated child-process failure -- stderr=' + r.stderr); + assert.ok(!(r.stdout || '').includes('SHOULD_NOT_REACH_HERE_IF_STRICT_MODE_WORKS'), + 'the .then() scheduled after the rejection must never have run once the process crashed'); }); + clearTimeout(watchdog); + + const missing = EXPECTED_SCENARIOS.filter((l) => !ranScenarios.has(l)); + if (missing.length > 0) { + failed++; + console.error('\n✗ INCOMPLETE SUITE: scenario(s) ' + missing.join(', ') + ' never ran to completion (expected exactly ' + + EXPECTED_SCENARIOS.join(', ') + ')'); + } + console.log('\n=== Summary ==='); + console.log(' Ran: ' + Array.from(ranScenarios).sort().join(', ') + ' (' + ranScenarios.size + '/' + EXPECTED_SCENARIOS.length + ')'); console.log(' Passed: ' + passed); console.log(' Failed: ' + failed); - console.log('\napi()-inflight-cleanup-rejection ' + (failed === 0 ? 'PASS' : 'FAIL')); - process.exitCode = failed === 0 ? 0 : 1; + const complete = missing.length === 0; + console.log('\napi()-inflight-cleanup-rejection ' + (failed === 0 && complete ? 'PASS' : 'FAIL')); + process.exitCode = (failed === 0 && complete) ? 0 : 1; })(); From 98fbdbe29e93be32722f78bda1e44c58119aaed0 Mon Sep 17 00:00:00 2001 From: Dennis Jakobsen Date: Mon, 14 Sep 2026 17:13:31 +0200 Subject: [PATCH 3/4] fix(test): close scenario H's false-pass and mislabelled child timeouts Scenario H asserted only that stderr *contained* the marker string, but when a child crashes for any reason (ReferenceError, SyntaxError, etc.) Node echoes the offending source line -- which itself contains the marker -- into stderr. That let H "pass" even for crashes unrelated to the deliberate unhandled rejection the scenario exists to detect. Replace it with an exact `^Error: $` (multiline) match against the thrown-message line, plus explicit signal/status checks so a signal kill (e.g. our own timeout) can never masquerade as the real crash. Both C's and H's spawnSync calls used only the default SIGTERM on timeout, which a misbehaving child can ignore and become orphaned, and a timed-out call surfaced as a generic "failed to spawn: ETIMEDOUT" message rather than a clear timeout. Add killSignal: 'SIGKILL' to both and check `error.code === 'ETIMEDOUT'` before the generic spawn-error assertion. Finally, reword the watchdog's comment/message: it is an event-loop safety bound for async stalls only, and firing does not mean "the loop was otherwise alive" (it also fires when it's the sole handle); make clear it cannot interrupt spawnSync or other synchronous blocking, so an outer runner-level timeout remains the real backstop for that case. Co-Authored-By: Claude Sonnet 5 --- test-app-api-inflight-cleanup-rejection.js | 61 ++++++++++++++++------ 1 file changed, 46 insertions(+), 15 deletions(-) diff --git a/test-app-api-inflight-cleanup-rejection.js b/test-app-api-inflight-cleanup-rejection.js index 7883eb081..374b27329 100644 --- a/test-app-api-inflight-cleanup-rejection.js +++ b/test-app-api-inflight-cleanup-rejection.js @@ -45,8 +45,14 @@ * 2. A watchdog `setTimeout` (not unref'd) is armed for the whole * run's duration. A real, non-unref'd timer is a pending macrotask, * so it keeps the event loop alive even if some other promise - * chain stalls -- Node cannot silently drain and exit while it is - * pending. If the suite hasn't finished by the deadline, the + * chain stalls -- Node cannot silently drain and exit while this + * timer is still the sole reason it stays alive. This is strictly + * an event-loop-based safety bound for ASYNC stalls: it cannot + * interrupt spawnSync (used by scenarios C and H) or any other + * synchronous blocking in this main process, because a blocked + * main thread never gets to run the timer's callback either -- + * an outer runner-level timeout remains the only backstop for + * that case. If the suite hasn't finished by the deadline, the * watchdog itself fails loudly and exits 1. It is cleared on the * normal completion path, so a healthy run's timing is unaffected. */ @@ -64,11 +70,13 @@ const WATCHDOG_MS = 15000; const watchdog = setTimeout(() => { console.error( '\n✗ WATCHDOG: the suite did not finish within ' + WATCHDOG_MS + 'ms. ' + - 'A real, non-unref\'d timer (this one) is a pending macrotask, so this ' + - 'firing means the event loop was otherwise still alive -- something is ' + - 'genuinely hung (not the historical "silent early exit 0" failure mode, ' + - 'which this timer separately prevents just by existing). Failing loudly ' + - 'instead of hanging CI indefinitely.' + 'This is an event-loop-based safety bound for an ASYNC stall (e.g. a ' + + 'permanently-pending promise) -- it cannot interrupt spawnSync or any ' + + 'other synchronous blocking in this main process, so it firing means ' + + 'something is genuinely stuck in async code (not the historical ' + + '"silent early exit 0" failure mode, which this timer separately ' + + 'prevents just by existing). Failing loudly instead of hanging CI ' + + 'indefinitely.' ); process.exitCode = 1; process.exit(1); @@ -238,7 +246,11 @@ function buildScenarioCChildScript(appJsPath) { await checkAsync('C. A handled request failure produces NO additional unhandled rejection (child process, --unhandled-rejections=strict)', async () => { const script = buildScenarioCChildScript(APP_JS_PATH); - const r = spawnSync(process.execPath, ['--unhandled-rejections=strict', '-e', script], { timeout: 10000, encoding: 'utf8' }); + const r = spawnSync(process.execPath, ['--unhandled-rejections=strict', '-e', script], + { timeout: 10000, killSignal: 'SIGKILL', encoding: 'utf8' }); + assert.ok(!(r.error && r.error.code === 'ETIMEDOUT'), + 'child process TIMED OUT after 10000ms and was killed with SIGKILL -- it never reached completion ' + + '(this is a hang in the child, not an unhandled-rejection failure)'); assert.strictEqual(r.error, undefined, 'child process failed to spawn: ' + (r.error && r.error.message)); assert.strictEqual(r.signal, null, @@ -326,15 +338,34 @@ function buildScenarioCChildScript(appJsPath) { for (let i = 0; i < 10; i++) p = p.then(() => new Promise((r) => setImmediate(r))); p.then(() => { console.log('SHOULD_NOT_REACH_HERE_IF_STRICT_MODE_WORKS'); }); `; - const r = spawnSync(process.execPath, ['--unhandled-rejections=strict', '-e', script], { timeout: 10000, encoding: 'utf8' }); + const r = spawnSync(process.execPath, ['--unhandled-rejections=strict', '-e', script], + { timeout: 10000, killSignal: 'SIGKILL', encoding: 'utf8' }); + assert.ok(!(r.error && r.error.code === 'ETIMEDOUT'), + 'child process TIMED OUT after 10000ms and was killed with SIGKILL -- it never reached completion ' + + '(this is a hang in the child, not evidence for or against the strict-mode technique)'); assert.strictEqual(r.error, undefined, 'child process failed to spawn: ' + (r.error && r.error.message)); - assert.notStrictEqual(r.status, 0, - 'expected the deliberate unhandled rejection to make the child exit non-zero under --unhandled-rejections=strict -- ' + - 'got status=' + r.status + ' (if this is 0, the harness technique used in scenario C cannot be trusted)'); - assert.ok((r.stderr || '').includes(marker), - 'expected the crash to be caused SPECIFICALLY by our deliberate rejection (stderr should mention "' + marker + '"), ' + - 'not an unrelated child-process failure -- stderr=' + r.stderr); + assert.strictEqual(r.signal, null, + 'child process was killed by a signal (' + r.signal + ') instead of exiting normally -- a signal kill ' + + '(including our own 10s timeout SIGKILL) must never be mistaken for the deliberate-rejection crash'); + assert.ok(typeof r.status === 'number' && r.status !== 0, + 'expected the deliberate unhandled rejection to make the child exit with a numeric non-zero status under ' + + '--unhandled-rejections=strict -- got status=' + JSON.stringify(r.status) + ' (null would mean the process ' + + 'was killed rather than exiting on its own, and must not count as a pass)'); + // A substring check here is not enough: when the child crashes for ANY + // reason (a typo, a syntax error), Node echoes the OFFENDING SOURCE LINE + // to stderr, and the script line above containing `${marker}` would + // itself satisfy a plain `.includes(marker)` check even though no + // deliberate-rejection crash occurred. Require the exact thrown-message + // line instead -- `Error: ` alone on its own line -- which only + // appears when Node prints the uncaught exception's message, not when it + // is merely quoting a source line. + const escapedMarker = marker.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const thrownMessageLine = new RegExp('^Error: ' + escapedMarker + '$', 'm'); + assert.ok(thrownMessageLine.test(r.stderr || ''), + 'expected the crash to be caused SPECIFICALLY by our deliberate rejection -- stderr should contain the exact ' + + 'thrown-message line "Error: ' + marker + '" on its own line, not merely mention the marker (e.g. by quoting ' + + 'the source line for an unrelated crash) -- stderr=' + r.stderr); assert.ok(!(r.stdout || '').includes('SHOULD_NOT_REACH_HERE_IF_STRICT_MODE_WORKS'), 'the .then() scheduled after the rejection must never have run once the process crashed'); }); From 340615953978fb7eccf62c236e7950530cf3ead3 Mon Sep 17 00:00:00 2001 From: Dennis Jakobsen Date: Mon, 14 Sep 2026 17:28:52 +0200 Subject: [PATCH 4/4] test: register test-app-api-inflight-cleanup-rejection.js in CI lists Adds the already-reviewed test-app-api-inflight-cleanup-rejection.js to test-all.sh and the deploy.yml "Run JS unit tests (packet-filter)" step, so it actually runs in CI. No other lines changed in either file. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/deploy.yml | 1 + test-all.sh | 1 + 2 files changed, 2 insertions(+) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 1e5c83a5f..a86dd21fb 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -104,6 +104,7 @@ jobs: node test-packet-filter-time.js node test-confidence-indicator.js node test-1659-analytics-warmup.js + node test-app-api-inflight-cleanup-rejection.js node test-channels-merge-1498-unit.js node test-issue-1518-home-url.js node test-channel-decrypt-insecure-context.js diff --git a/test-all.sh b/test-all.sh index ca4c3847d..b89a1db68 100755 --- a/test-all.sh +++ b/test-all.sh @@ -14,6 +14,7 @@ node test-packet-filter-ux.js node test-aging.js node test-issue-1065-gesture-hints-gates.js node test-frontend-helpers.js +node test-app-api-inflight-cleanup-rejection.js node test-privacy-page.js node test-nav-dynamic-link-lifecycle.js node test-nav-first-load-fit.js