From 7a5d0c3d492797994a15223b5f617c1d1a8f4dbd Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 14:19:45 +0000 Subject: [PATCH 1/9] Harden against symbol lookup floods - Grow the negative (missing-symbol) LRU cache from 10k to 500k entries and give entries a 1 hour TTL so late-uploaded symbols reappear - Cap concurrent proxied upstream requests (default 100, tunable via MAX_UPSTREAM_CONCURRENCY) and immediately shed excess load with a 503 and Retry-After instead of queueing behind the Heroku router - Dedupe concurrent in-flight lookups for the same path: followers wait for the first fetch to settle and answer from the negative cache - Emit Cache-Control headers so a CDN can absorb repeat traffic: public max-age=3600 on 404s, public max-age=604800 immutable on 200s without an upstream header, and no-store on symbolicator redirects --- src/index.ts | 66 ++++++++++++++++++++++++++-- test/helpers.js | 7 +-- test/server.test.js | 102 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 168 insertions(+), 7 deletions(-) diff --git a/src/index.ts b/src/index.ts index 72eb758..9f63c99 100644 --- a/src/index.ts +++ b/src/index.ts @@ -5,10 +5,25 @@ import httpProxy from 'http-proxy'; import LRU from 'lru-cache'; import * as url from 'url'; -const { PATH_PREFIX, TARGET_HOST } = process.env; +const { PATH_PREFIX, TARGET_HOST, MAX_UPSTREAM_CONCURRENCY } = process.env; assert(TARGET_HOST, 'TARGET_HOST is defined'); +// How long a "this symbol does not exist" answer stays valid, both in our +// in-memory cache and in any CDN/client honoring Cache-Control. Kept short-ish +// so symbols uploaded later (e.g. new releases) aren't hidden forever. +const MISSING_SYMBOL_TTL_SECONDS = 60 * 60; +const MISSING_CACHE_CONTROL = `public, max-age=${MISSING_SYMBOL_TTL_SECONDS}`; +// Symbol files are immutable for a given debug-id path, so hits can be cached +// aggressively by CDNs/clients. +const HIT_CACHE_CONTROL = 'public, max-age=604800, immutable'; + +// Cap on concurrent proxied upstream requests. Beyond this we shed load +// immediately with a 503 instead of queueing, so the dyno's backlog stays +// shallow during floods. +const UPSTREAM_CONCURRENCY_LIMIT = parseInt(MAX_UPSTREAM_CONCURRENCY || '', 10) || 100; +const RETRY_AFTER_SECONDS = 30; + const TARGET_URL = url.format({ protocol: 'https:', slashes: true, @@ -36,9 +51,15 @@ REPLACEMENTS.push([/\/c:\\projects\\src\\out\\default\\/g, '/']); REPLACEMENTS.push([/\/c%3a%5cprojects%5csrc%5cout%5cdefault%5c/g, '/']); const missingSymbolCache = new LRU({ - max: 10000, + max: 500_000, + maxAge: MISSING_SYMBOL_TTL_SECONDS * 1000, }); +// Proxied requests currently awaiting an upstream response, keyed by rewritten +// path. Used to dedupe identical concurrent lookups and to enforce the +// upstream concurrency cap. +const inFlightRequests = new Map>(); + function incomingPathToProxyPath(path: string): string { // symstore.exe and symsrv.dll don't always agree on the case of the path to a // given symbol file. Since our artifact URLs are case-sensitive, this causes symbol @@ -78,8 +99,12 @@ proxy.on('proxyReq', (proxyReq, request, response, options) => { if (args[0] == 403) { missingSymbolCache.set(proxyReq.path, true); args[0] = 404; + response.setHeader('Cache-Control', MISSING_CACHE_CONTROL); } else { missingSymbolCache.set(proxyReq.path, false); + if (args[0] == 200 && !response.getHeader('cache-control')) { + response.setHeader('Cache-Control', HIT_CACHE_CONTROL); + } } return originalWriteHead.apply(response, args); }; @@ -114,16 +139,49 @@ http.createServer((req, res) => { host: TARGET_HOST, pathname: cacheKey, })); + res.setHeader('Cache-Control', 'no-store'); return res.writeHead(302).end(); } if (missingSymbolCache.get(cacheKey)) { - return res.writeHead(404).end(); + return res.writeHead(404, { 'Cache-Control': MISSING_CACHE_CONTROL }).end(); } - proxy.web(req, res, { target: TARGET_URL }); + // If an identical lookup is already being proxied, wait for it to settle + // rather than launching a duplicate upstream fetch. If it negative-cached + // the path we can answer 404 for free, otherwise proxy as usual. + const inFlight = inFlightRequests.get(cacheKey); + if (inFlight) { + inFlight.then(() => { + if (missingSymbolCache.get(cacheKey)) { + return res.writeHead(404, { 'Cache-Control': MISSING_CACHE_CONTROL }).end(); + } + proxyToUpstream(req, res, cacheKey); + }); + return; + } + + proxyToUpstream(req, res, cacheKey); }).listen(process.env.PORT || 8080); +function proxyToUpstream(req: http.IncomingMessage, res: http.ServerResponse, cacheKey: string) { + if (inFlightRequests.size >= UPSTREAM_CONCURRENCY_LIMIT) { + // Shed load immediately instead of queueing behind a saturated upstream, + // otherwise the router backlog fills up and everyone gets H11 503s. + res.setHeader('Retry-After', String(RETRY_AFTER_SECONDS)); + return res.writeHead(503).end('Too many concurrent symbol requests, retry later'); + } + + inFlightRequests.set(cacheKey, new Promise((resolve) => { + res.on('close', () => { + inFlightRequests.delete(cacheKey); + resolve(); + }); + })); + + proxy.web(req, res, { target: TARGET_URL }); +} + process.on('uncaughtException', (err) => { // Avoid process dieing on uncaughtException console.error(err); diff --git a/test/helpers.js b/test/helpers.js index eea6b85..61bd499 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -77,11 +77,12 @@ function startUpstream(handler) { }); } -async function startSymbolServer({ targetHost, pathPrefix } = {}) { +async function startSymbolServer({ targetHost, pathPrefix, env: extraEnv } = {}) { const port = await getFreePort(); const env = { ...process.env, + ...extraEnv, TARGET_HOST: targetHost, PORT: String(port), // http-proxy uses the default https agent; NODE_EXTRA_CA_CERTS is the @@ -134,14 +135,14 @@ async function startSymbolServer({ targetHost, pathPrefix } = {}) { // Spawn an upstream + symbol-server pair and register cleanup with the test // context. Returns { server, upstream }. -async function startProxy(t, { handler, pathPrefix } = {}) { +async function startProxy(t, { handler, pathPrefix, env } = {}) { const upstream = await startUpstream(handler || ((req, res) => { res.writeHead(200); res.end('ok'); })); t.after(() => upstream.close()); - const server = await startSymbolServer({ targetHost: upstream.host, pathPrefix }); + const server = await startSymbolServer({ targetHost: upstream.host, pathPrefix, env }); t.after(() => server.stop()); return { server, upstream }; diff --git a/test/server.test.js b/test/server.test.js index 10ea182..e32f561 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -177,6 +177,108 @@ test('upstream non-403 errors are passed through and not cached as missing', asy assert.equal(calls, 2); }); +test('negative-cache 404s carry a public Cache-Control header', async (t) => { + const { server } = await startProxy(t, { + handler: (req, res) => { + res.writeHead(403); + res.end(); + }, + }); + + const first = await request(server.port, '/missing/foo.pdb/abc/foo.pdb'); + assert.equal(first.statusCode, 404); + assert.equal(first.headers['cache-control'], 'public, max-age=3600'); + + // Served from the negative cache without contacting upstream. + const second = await request(server.port, '/missing/foo.pdb/abc/foo.pdb'); + assert.equal(second.statusCode, 404); + assert.equal(second.headers['cache-control'], 'public, max-age=3600'); +}); + +test('successful 200s get a long immutable Cache-Control when upstream sends none', async (t) => { + const { server } = await startProxy(t, { + handler: (req, res) => { + res.writeHead(200); + res.end('SYMBOL-DATA'); + }, + }); + + const res = await request(server.port, '/foo/bar.pdb/abc/foo.pdb'); + assert.equal(res.statusCode, 200); + assert.equal(res.headers['cache-control'], 'public, max-age=604800, immutable'); +}); + +test('upstream Cache-Control on 200s is preserved', async (t) => { + const { server } = await startProxy(t, { + handler: (req, res) => { + res.writeHead(200, { 'cache-control': 'public, max-age=60' }); + res.end('SYMBOL-DATA'); + }, + }); + + const res = await request(server.port, '/foo/bar.pdb/abc/foo.pdb'); + assert.equal(res.statusCode, 200); + assert.equal(res.headers['cache-control'], 'public, max-age=60'); +}); + +test('redirect responses are not cacheable', async (t) => { + const server = await startSymbolServer({ targetHost: 'symbols.example.test' }); + t.after(() => server.stop()); + + const res = await request(server.port, '/Foo/Bar', { + 'user-agent': 'symbolicator/1.2.3', + }); + assert.equal(res.statusCode, 302); + assert.equal(res.headers['cache-control'], 'no-store'); +}); + +test('sheds load with 503 + Retry-After above the upstream concurrency cap', async (t) => { + let releaseFirst; + const firstHeld = new Promise((resolve) => { releaseFirst = resolve; }); + const { server, upstream } = await startProxy(t, { + env: { MAX_UPSTREAM_CONCURRENCY: '1' }, + handler: async (req, res) => { + await firstHeld; + res.writeHead(200); + res.end('ok'); + }, + }); + + const first = request(server.port, '/held/foo.pdb/abc/foo.pdb'); + // Wait until the first request has actually reached upstream. + while (upstream.requests.length === 0) { + await new Promise((resolve) => setTimeout(resolve, 10)); + } + + const shed = await request(server.port, '/other/foo.pdb/abc/foo.pdb'); + assert.equal(shed.statusCode, 503); + assert.equal(shed.headers['retry-after'], '30'); + assert.equal(upstream.requests.length, 1, 'shed request should not reach upstream'); + + releaseFirst(); + const held = await first; + assert.equal(held.statusCode, 200); +}); + +test('concurrent requests for the same missing path only hit upstream once', async (t) => { + const { server, upstream } = await startProxy(t, { + handler: (req, res) => { + setTimeout(() => { + res.writeHead(403); + res.end(); + }, 100); + }, + }); + + const [first, second] = await Promise.all([ + request(server.port, '/dup/foo.pdb/abc/foo.pdb'), + request(server.port, '/dup/foo.pdb/abc/foo.pdb'), + ]); + assert.equal(first.statusCode, 404); + assert.equal(second.statusCode, 404); + assert.equal(upstream.requests.length, 1, 'duplicate lookup should not reach upstream'); +}); + test('proxy returns 500 with error ID when upstream is unreachable', async (t) => { const server = await startSymbolServer({ targetHost: '127.0.0.1:1' }); t.after(() => server.stop()); From 552d9f0a832fe89c9de9da532785766c5c9ddbdf Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 15:04:51 +0000 Subject: [PATCH 2/9] Fix concurrency cap bypass for same-path waiters and shrink negative cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixes from review of the flood-hardening changes: - The upstream concurrency cap was enforced against the in-flight dedup Map's size, but every dedup waiter that woke after its leader settled proxied under the same map key. Map.size stayed at 1 while N upstream requests were active (observed 6 simultaneous with a cap of 2), and the first response to close deleted the shared entry out from under the rest. The cap is now enforced against a dedicated counter that is incremented when a proxy actually starts and settled exactly once on close/error; woken waiters re-check the cap and are shed with 503 + Retry-After when at capacity. The Map is kept for dedup only, and an entry is only removed by the request that registered it. - The negative cache stored an entry for every upstream response, including useless `false` markers for hits and errors, and was bounded at 500k entries (~190 MiB of V8 heap when full with representative paths — dangerous on a dyno already seeing R14s). It now stores entries only for genuine misses (the 403 -> 404 case) and is bounded by bytes instead of entry count: 32 MiB of key bytes via lru-cache@6's length-based accounting, keeping the 1h TTL. Adds a regression test that reproduces the same-path stampede against a capped upstream and asserts the cap holds and excess waiters are shed. --- src/index.ts | 65 ++++++++++++++++++++++++++++++++++----------- test/server.test.js | 62 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 15 deletions(-) diff --git a/src/index.ts b/src/index.ts index 9f63c99..23ae11a 100644 --- a/src/index.ts +++ b/src/index.ts @@ -50,16 +50,30 @@ for (const appName of APPS_TO_ALIAS) { REPLACEMENTS.push([/\/c:\\projects\\src\\out\\default\\/g, '/']); REPLACEMENTS.push([/\/c%3a%5cprojects%5csrc%5cout%5cdefault%5c/g, '/']); +// Bound the negative cache by total bytes rather than entry count so its +// worst-case memory footprint stays predictable on a small dyno. In +// lru-cache@6, providing a `length` calculator makes `max` a total-length +// budget; we charge each entry the string length of its path key (the boolean +// value is negligible). Rewritten symbol paths are ~60-100 bytes, so 32 MiB is +// roughly 300-500k entries worst case, but bounded in bytes either way. +const MISSING_CACHE_MAX_BYTES = 32 * 1024 * 1024; + const missingSymbolCache = new LRU({ - max: 500_000, + max: MISSING_CACHE_MAX_BYTES, + length: (_value, key) => (key as string).length, maxAge: MISSING_SYMBOL_TTL_SECONDS * 1000, }); // Proxied requests currently awaiting an upstream response, keyed by rewritten -// path. Used to dedupe identical concurrent lookups and to enforce the -// upstream concurrency cap. +// path. Used only to dedupe identical concurrent lookups — NOT for the +// concurrency cap: multiple proxied requests for the same path share a single +// map entry, so Map.size undercounts. const inFlightRequests = new Map>(); +// Number of proxied upstream requests actually in flight right now. This is +// what the concurrency cap is enforced against. +let activeUpstreamRequests = 0; + function incomingPathToProxyPath(path: string): string { // symstore.exe and symsrv.dll don't always agree on the case of the path to a // given symbol file. Since our artifact URLs are case-sensitive, this causes symbol @@ -97,14 +111,14 @@ proxy.on('proxyReq', (proxyReq, request, response, options) => { const originalWriteHead = response.writeHead; response.writeHead = (...args: [number, any]) => { if (args[0] == 403) { + // Only genuine misses go in the negative cache. Hits and transport + // errors used to be stored as `false`, which answered no query the + // cache's absence wouldn't, but still consumed an LRU slot each. missingSymbolCache.set(proxyReq.path, true); args[0] = 404; response.setHeader('Cache-Control', MISSING_CACHE_CONTROL); - } else { - missingSymbolCache.set(proxyReq.path, false); - if (args[0] == 200 && !response.getHeader('cache-control')) { - response.setHeader('Cache-Control', HIT_CACHE_CONTROL); - } + } else if (args[0] == 200 && !response.getHeader('cache-control')) { + response.setHeader('Cache-Control', HIT_CACHE_CONTROL); } return originalWriteHead.apply(response, args); }; @@ -149,7 +163,9 @@ http.createServer((req, res) => { // If an identical lookup is already being proxied, wait for it to settle // rather than launching a duplicate upstream fetch. If it negative-cached - // the path we can answer 404 for free, otherwise proxy as usual. + // the path we can answer 404 for free, otherwise proxy as usual — + // proxyToUpstream re-checks the concurrency cap when we wake, so a stampede + // of same-path waiters is shed instead of all proxying at once. const inFlight = inFlightRequests.get(cacheKey); if (inFlight) { inFlight.then(() => { @@ -165,19 +181,38 @@ http.createServer((req, res) => { }).listen(process.env.PORT || 8080); function proxyToUpstream(req: http.IncomingMessage, res: http.ServerResponse, cacheKey: string) { - if (inFlightRequests.size >= UPSTREAM_CONCURRENCY_LIMIT) { + if (activeUpstreamRequests >= UPSTREAM_CONCURRENCY_LIMIT) { // Shed load immediately instead of queueing behind a saturated upstream, // otherwise the router backlog fills up and everyone gets H11 503s. res.setHeader('Retry-After', String(RETRY_AFTER_SECONDS)); return res.writeHead(503).end('Too many concurrent symbol requests, retry later'); } - inFlightRequests.set(cacheKey, new Promise((resolve) => { - res.on('close', () => { - inFlightRequests.delete(cacheKey); + activeUpstreamRequests++; + const inFlight = new Promise((resolve) => { + // Both 'close' and 'error' can fire for the same response; settle exactly + // once so the active count can never be decremented twice. + let settled = false; + const settle = () => { + if (settled) return; + settled = true; + activeUpstreamRequests--; + // Several proxied requests for the same path can coexist (dedup waiters + // that woke below the cap); only the one registered in the map may + // remove the entry, or a later request's dedup entry would be dropped + // while it is still in flight. + if (inFlightRequests.get(cacheKey) === inFlight) { + inFlightRequests.delete(cacheKey); + } resolve(); - }); - })); + }; + res.on('close', settle); + res.on('error', settle); + }); + + if (!inFlightRequests.has(cacheKey)) { + inFlightRequests.set(cacheKey, inFlight); + } proxy.web(req, res, { target: TARGET_URL }); } diff --git a/test/server.test.js b/test/server.test.js index e32f561..f75776e 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -260,6 +260,68 @@ test('sheds load with 503 + Retry-After above the upstream concurrency cap', asy assert.equal(held.statusCode, 200); }); +test('same-path dedup waiters cannot bypass the upstream concurrency cap', async (t) => { + // Regression test: waiters queued behind an in-flight leader used to all + // call proxyToUpstream when the leader settled. Each overwrote the same + // in-flight map key, so the Map.size-based cap check saw 1 while N upstream + // requests were actually active (observed: 6 with a cap of 2). + let active = 0; + let maxActive = 0; + let phase = 'leader'; + let releaseLeader; + const leaderHeld = new Promise((resolve) => { releaseLeader = resolve; }); + let releaseWaiters; + const waitersHeld = new Promise((resolve) => { releaseWaiters = resolve; }); + + const { server, upstream } = await startProxy(t, { + env: { MAX_UPSTREAM_CONCURRENCY: '2' }, + handler: async (req, res) => { + active += 1; + maxActive = Math.max(maxActive, active); + if (phase === 'leader') await leaderHeld; else await waitersHeld; + res.writeHead(200); + res.end('ok'); + active -= 1; + }, + }); + + const PATH = '/stampede/foo.pdb/abc/foo.pdb'; + const leader = request(server.port, PATH); + while (upstream.requests.length === 0) { + await new Promise((resolve) => setTimeout(resolve, 10)); + } + + // Queue six identical lookups; all should dedup-wait on the leader. + phase = 'waiters'; + const waiters = []; + for (let i = 0; i < 6; i++) waiters.push(request(server.port, PATH)); + await new Promise((resolve) => setTimeout(resolve, 200)); + assert.equal(upstream.requests.length, 1, 'waiters must not reach upstream while leader is in flight'); + + // Leader succeeds; woken waiters re-check the cap, so only two may proxy. + releaseLeader(); + const deadline = Date.now() + 2000; + while (upstream.requests.length < 3 && Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, 10)); + } + // Grace period to catch any waiters that slipped past the cap. + await new Promise((resolve) => setTimeout(resolve, 200)); + releaseWaiters(); + + const leaderRes = await leader; + const waiterRes = await Promise.all(waiters); + + assert.equal(leaderRes.statusCode, 200); + assert.ok(maxActive <= 2, `at most 2 simultaneous upstream requests allowed, saw ${maxActive}`); + assert.equal(upstream.requests.length, 3, 'leader + at most cap-many waiters may reach upstream'); + + const okCount = waiterRes.filter((r) => r.statusCode === 200).length; + const shed = waiterRes.filter((r) => r.statusCode === 503); + assert.equal(okCount, 2, 'exactly cap-many waiters should be proxied'); + assert.equal(shed.length, 4, 'remaining waiters should be shed'); + for (const r of shed) assert.equal(r.headers['retry-after'], '30'); +}); + test('concurrent requests for the same missing path only hit upstream once', async (t) => { const { server, upstream } = await startProxy(t, { handler: (req, res) => { From 5dcfbc5fb5c16f94e036223b9709660e45e61a48 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 15:27:53 +0000 Subject: [PATCH 3/9] Fix upstream slot leak for canceled dedup waiters and account real heap in negative cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixes from PR review: 1. A dedup waiter whose client disconnected while waiting on the leader would still call proxyToUpstream on wake. Its response's 'close' event had already fired before the settle listeners were registered, so the incremented activeUpstreamRequests slot could never be released. With MAX_UPSTREAM_CONCURRENCY=1 a single canceled waiter turned every subsequent request into a 503 until process restart. Now: requests whose client is already gone are dropped before touching the counter (fresh requests and woken waiters alike), and as a belt-and-braces guard, if the client vanishes between that check and listener registration, settle() is invoked by hand — it is idempotent, so a double decrement is impossible. 2. The negative cache's 32 MiB budget charged only key.length, but each entry really costs ~480-510 bytes of heap (lru-cache node + Map entry + string headers; measured with --expose-gc, 96-char keys, filled to steady-state eviction: 349,525 entries / ~160 MiB heapUsed). Charge a measured 384-byte per-entry overhead so a full cache is ~70k entries and ~34 MiB of real heap. Test helpers now sever lingering upstream connections on close so a leaked proxied connection fails the new regression test instead of hanging the runner's after-hooks. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01ToXSyZGzHfwtNoF6mWoJUA --- src/index.ts | 59 +++++++++++++++++++++++++++++++++++++++------ test/helpers.js | 3 +++ test/server.test.js | 44 +++++++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 8 deletions(-) diff --git a/src/index.ts b/src/index.ts index 23ae11a..94cf7a7 100644 --- a/src/index.ts +++ b/src/index.ts @@ -53,14 +53,20 @@ REPLACEMENTS.push([/\/c%3a%5cprojects%5csrc%5cout%5cdefault%5c/g, '/']); // Bound the negative cache by total bytes rather than entry count so its // worst-case memory footprint stays predictable on a small dyno. In // lru-cache@6, providing a `length` calculator makes `max` a total-length -// budget; we charge each entry the string length of its path key (the boolean -// value is negligible). Rewritten symbol paths are ~60-100 bytes, so 32 MiB is -// roughly 300-500k entries worst case, but bounded in bytes either way. +// budget. Charging only the key's string length badly undercounts real heap: +// each entry also costs an lru-cache linked-list node, a Map entry, and V8 +// string/object headers. Measured with node --expose-gc on lru-cache@6 using +// representative 96-char keys filled to steady-state eviction: ~480-510 bytes +// of heapUsed per entry, i.e. roughly 384 bytes of overhead beyond the key +// itself. Charging key.length alone allowed ~350k entries and ~160 MiB of +// real heap against this 32 MiB budget; charging the measured overhead keeps +// a full cache at ~70k entries and ~34 MiB of measured heap. const MISSING_CACHE_MAX_BYTES = 32 * 1024 * 1024; +const MISSING_CACHE_ENTRY_OVERHEAD_BYTES = 384; const missingSymbolCache = new LRU({ max: MISSING_CACHE_MAX_BYTES, - length: (_value, key) => (key as string).length, + length: (_value, key) => (key as string).length + MISSING_CACHE_ENTRY_OVERHEAD_BYTES, maxAge: MISSING_SYMBOL_TTL_SECONDS * 1000, }); @@ -169,6 +175,13 @@ http.createServer((req, res) => { const inFlight = inFlightRequests.get(cacheKey); if (inFlight) { inFlight.then(() => { + // The client may have hung up while we waited on the leader. Its + // response 'close' event has already fired by now, so proxying would + // register settle listeners that never run and leak an upstream slot + // forever (proxyToUpstream re-checks this, but bail early and skip the + // cache lookup too). Dropped waiters touch no counters, so there is + // nothing to clean up. + if (clientGone(req, res)) return; if (missingSymbolCache.get(cacheKey)) { return res.writeHead(404, { 'Cache-Control': MISSING_CACHE_CONTROL }).end(); } @@ -180,7 +193,25 @@ http.createServer((req, res) => { proxyToUpstream(req, res, cacheKey); }).listen(process.env.PORT || 8080); +// True when the client that issued this request can no longer receive a +// response: its socket is gone (disconnect — note 'close' fires on the +// response even before headers are written) or the response already ended. +function clientGone(req: http.IncomingMessage, res: http.ServerResponse): boolean { + return ( + req.destroyed || + res.destroyed || + res.writableEnded || + !res.socket || + res.socket.destroyed + ); +} + function proxyToUpstream(req: http.IncomingMessage, res: http.ServerResponse, cacheKey: string) { + // Never proxy on behalf of a client that already disconnected: its 'close' + // event has already fired, so the settle listeners below would never run + // and the upstream slot would leak until process restart. + if (clientGone(req, res)) return; + if (activeUpstreamRequests >= UPSTREAM_CONCURRENCY_LIMIT) { // Shed load immediately instead of queueing behind a saturated upstream, // otherwise the router backlog fills up and everyone gets H11 503s. @@ -189,11 +220,14 @@ function proxyToUpstream(req: http.IncomingMessage, res: http.ServerResponse, ca } activeUpstreamRequests++; + // Both 'close' and 'error' can fire for the same response, and settle() is + // additionally called by hand below when the client disconnected before the + // listeners were registered; settle exactly once so the active count can + // never be decremented twice. + let settled = false; + let settle!: () => void; const inFlight = new Promise((resolve) => { - // Both 'close' and 'error' can fire for the same response; settle exactly - // once so the active count can never be decremented twice. - let settled = false; - const settle = () => { + settle = () => { if (settled) return; settled = true; activeUpstreamRequests--; @@ -214,6 +248,15 @@ function proxyToUpstream(req: http.IncomingMessage, res: http.ServerResponse, ca inFlightRequests.set(cacheKey, inFlight); } + // 'close' fires at most once. If the client vanished between the clientGone + // check at the top of this function and the listener registration above, it + // has already fired and never will again — settle by hand and skip the + // upstream fetch entirely. + if (clientGone(req, res)) { + settle(); + return; + } + proxy.web(req, res, { target: TARGET_URL }); } diff --git a/test/helpers.js b/test/helpers.js index 61bd499..17939a5 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -70,6 +70,9 @@ function startUpstream(handler) { requests, close: () => new Promise((res) => { + // Sever any connections still open (e.g. leaked by a bug under + // test) so close() cannot hang the test runner's after-hooks. + server.closeAllConnections(); server.close(() => res()); }), }); diff --git a/test/server.test.js b/test/server.test.js index f75776e..21da283 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -1,5 +1,6 @@ 'use strict'; +const http = require('node:http'); const test = require('node:test'); const assert = require('node:assert/strict'); @@ -322,6 +323,49 @@ test('same-path dedup waiters cannot bypass the upstream concurrency cap', async for (const r of shed) assert.equal(r.headers['retry-after'], '30'); }); +test('a dedup waiter whose client disconnects mid-wait does not leak an upstream slot', async (t) => { + // Regression test: a same-path waiter used to call proxyToUpstream when the + // leader settled even if its own client had already hung up. The response's + // 'close' event had fired before the settle listeners were registered, so + // settle never ran and the incremented activeUpstreamRequests slot leaked + // forever. With a cap of 1 a single canceled waiter then turned every + // subsequent distinct-path request into a 503 until process restart. + const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); + let phase = 'leader'; + let releaseLeader; + const leaderHeld = new Promise((resolve) => { releaseLeader = resolve; }); + const { server, upstream } = await startProxy(t, { + env: { MAX_UPSTREAM_CONCURRENCY: '1' }, + handler: async (req, res) => { + if (phase === 'leader') await leaderHeld; + res.writeHead(200); + res.end('ok'); + }, + }); + + const PATH = '/held/foo.pdb/abc/foo.pdb'; + const leader = request(server.port, PATH); + while (upstream.requests.length === 0) await sleep(10); + + // Same-path waiter; destroy its client socket while it waits on the leader. + const waiter = http.request({ host: '127.0.0.1', port: server.port, path: PATH, method: 'GET' }); + waiter.on('error', () => {}); + waiter.end(); + await sleep(200); // let the server register it as a dedup waiter + waiter.destroy(); + await sleep(200); // let the server-side 'close' fire + + phase = 'done'; + releaseLeader(); + const leaderRes = await leader; + assert.equal(leaderRes.statusCode, 200); + await sleep(200); // let the canceled waiter wake and (previously) leak + + const probe = await request(server.port, '/distinct/bar.pdb/def/bar.pdb'); + assert.equal(probe.statusCode, 200, 'canceled waiter must not leak an upstream slot'); + assert.equal(upstream.requests.length, 2, 'only the leader and the probe should reach upstream'); +}); + test('concurrent requests for the same missing path only hit upstream once', async (t) => { const { server, upstream } = await startProxy(t, { handler: (req, res) => { From 38877cca36f83cd3648fac91e485a1ffff6e2f20 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 15:41:08 +0000 Subject: [PATCH 4/9] Cancel the upstream request when the client disconnects mid-proxy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The upstream slot counter and dedup promise settled when the downstream response closed, but http-proxy does not cancel the outgoing upstream request on its own: its req 'aborted' hook never fires on modern Node for requests whose body was already fully received (every GET). A client disconnecting mid-proxy therefore freed its slot and woke same-path dedup waiters while the upstream fetch was still running, bypassing the concurrency cap (with cap=1, a distinct second request reached upstream alongside the abandoned one, and upstream never saw an abort). Tie the lifecycle to the actual upstream request instead: the shared proxyReq hook maps each outgoing request back to its downstream request via a WeakMap, downstream 'close'/'error' now destroys the captured proxyReq, and the slot/dedup promise settle on the upstream request's 'close' — which fires both on normal completion and after an abort — so waiters can never wake into a still-occupied slot. If no proxyReq was captured by the time the client vanishes (never reached proxy.web, or http-proxy skipped the hook for Expect: 100-continue), settle immediately as before so the slot cannot leak. The proxy error handler no longer writes a 500 to a response whose client is already gone. Adds a regression test: cap=1, client disconnects mid-proxy — upstream must observe the abort and must never see two simultaneous active requests. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01ToXSyZGzHfwtNoF6mWoJUA --- src/index.ts | 87 +++++++++++++++++++++++++++++++++++++++++---- test/server.test.js | 59 ++++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 6 deletions(-) diff --git a/src/index.ts b/src/index.ts index 94cf7a7..9137f83 100644 --- a/src/index.ts +++ b/src/index.ts @@ -80,6 +80,31 @@ const inFlightRequests = new Map>(); // what the concurrency cap is enforced against. let activeUpstreamRequests = 0; +// Ties each proxied downstream request to the upstream request http-proxy +// opens for it. The shared 'proxyReq' hook below fires for every proxied +// request, so it must map each proxyReq back to the right downstream request; +// keying by the incoming request object does that without any cleanup +// bookkeeping (entries die with the request). +interface UpstreamLifecycle { + proxyReq: http.ClientRequest | null; + downstreamGone: boolean; + settle: () => void; +} + +const upstreamLifecycles = new WeakMap(); + +// Cancel the outgoing upstream request. destroy() on a request that already +// completed just tears down its (connection: close) socket, but be defensive: +// a throw here would bubble into an event handler and kill nothing gracefully. +function abortUpstreamRequest(proxyReq: http.ClientRequest) { + if (proxyReq.destroyed) return; + try { + proxyReq.destroy(); + } catch (err) { + console.error('Failed to abort upstream request:', err); + } +} + function incomingPathToProxyPath(path: string): string { // symstore.exe and symsrv.dll don't always agree on the case of the path to a // given symbol file. Since our artifact URLs are case-sensitive, this causes symbol @@ -128,6 +153,20 @@ proxy.on('proxyReq', (proxyReq, request, response, options) => { } return originalWriteHead.apply(response, args); }; + + const lifecycle = upstreamLifecycles.get(request); + if (lifecycle) { + lifecycle.proxyReq = proxyReq; + // The upstream slot and dedup promise settle on the UPSTREAM request's + // lifecycle, not the downstream response's: 'close' fires both when the + // proxied response has been fully read and when the request is destroyed + // or errors, so normal completion and client-abort cancellation route to + // the same idempotent settle. + proxyReq.on('close', lifecycle.settle); + // The client may have vanished between proxy.web() and the socket + // assignment that fires this event; cancel the upstream work right away. + if (lifecycle.downstreamGone) abortUpstreamRequest(proxyReq); + } }); proxy.on('error', (err, req, res) => { @@ -135,6 +174,11 @@ proxy.on('error', (err, req, res) => { console.error('Error:', errorId, 'Request:', req.url, err); + // A deliberately canceled upstream request (client disconnected mid-proxy) + // can surface its teardown error here; there is no one left to answer and + // writing headers to a closed/finished response would throw. + if (res.destroyed || res.writableEnded || res.headersSent) return; + res.writeHead(500, { 'Content-Type': 'text/plain' }); @@ -220,10 +264,10 @@ function proxyToUpstream(req: http.IncomingMessage, res: http.ServerResponse, ca } activeUpstreamRequests++; - // Both 'close' and 'error' can fire for the same response, and settle() is - // additionally called by hand below when the client disconnected before the - // listeners were registered; settle exactly once so the active count can - // never be decremented twice. + // settle() can be reached from several events (the upstream request's + // 'close', the downstream 'close'/'error' fallback below, and by hand when + // the client disconnected before the listeners were registered); settle + // exactly once so the active count can never be decremented twice. let settled = false; let settle!: () => void; const inFlight = new Promise((resolve) => { @@ -240,10 +284,41 @@ function proxyToUpstream(req: http.IncomingMessage, res: http.ServerResponse, ca } resolve(); }; - res.on('close', settle); - res.on('error', settle); }); + // Tie teardown to the actual upstream request rather than the downstream + // response alone: http-proxy does not cancel the outgoing request by itself + // when the client disconnects mid-proxy (its req 'aborted' hook never fires + // on modern Node for requests whose body was already fully received, i.e. + // every GET), so settling on downstream 'close' freed the slot and woke + // dedup waiters while the upstream fetch was still running — bypassing the + // cap. Instead, downstream 'close'/'error' destroys the upstream request, + // and the slot/dedup promise settle only once that request has ended or + // been aborted (its 'close' listener, registered in the proxyReq hook), so + // waiters can never wake into a still-occupied slot. + const lifecycle: UpstreamLifecycle = { proxyReq: null, downstreamGone: false, settle }; + upstreamLifecycles.set(req, lifecycle); + const onDownstreamGone = () => { + if (lifecycle.downstreamGone) return; + lifecycle.downstreamGone = true; + if (lifecycle.proxyReq) { + // Cancel the upstream work; settle fires when the destroyed request + // emits 'close'. (After a normal completion this destroy is a no-op on + // an already-finished request.) + abortUpstreamRequest(lifecycle.proxyReq); + } else { + // No upstream request was captured for this response — either we never + // reached proxy.web below, or http-proxy skipped the proxyReq event + // (it does for Expect: 100-continue requests). Nothing to cancel; + // settle now so the slot cannot leak. Should the capture still happen a + // tick later, the downstreamGone flag above makes it destroy the + // upstream request immediately. + settle(); + } + }; + res.on('close', onDownstreamGone); + res.on('error', onDownstreamGone); + if (!inFlightRequests.has(cacheKey)) { inFlightRequests.set(cacheKey, inFlight); } diff --git a/test/server.test.js b/test/server.test.js index 21da283..94f7367 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -366,6 +366,65 @@ test('a dedup waiter whose client disconnects mid-wait does not leak an upstream assert.equal(upstream.requests.length, 2, 'only the leader and the probe should reach upstream'); }); +test('client disconnect mid-proxy aborts upstream and frees the slot only after', async (t) => { + // Regression test: the slot counter and dedup promise used to settle when + // the DOWNSTREAM response closed, but http-proxy does not cancel the + // UPSTREAM request on its own (its req 'aborted' hook never fires for + // fully-received requests on modern Node). A client disconnecting mid-proxy + // therefore freed its slot while the upstream fetch kept running: with a + // cap of 1, a distinct second request then also reached upstream, which saw + // 2 simultaneous active requests and never observed an abort. + const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); + let active = 0; + let maxActive = 0; + let aborts = 0; + let release; + const held = new Promise((resolve) => { release = resolve; }); + const { server, upstream } = await startProxy(t, { + env: { MAX_UPSTREAM_CONCURRENCY: '1' }, + handler: async (req, res) => { + active += 1; + maxActive = Math.max(maxActive, active); + res.on('close', () => { + if (!res.writableEnded) aborts += 1; + active -= 1; + }); + await held; + if (!res.destroyed) { + res.writeHead(200); + res.end('ok'); + } + }, + }); + + // First request reaches the held upstream, then its client disconnects. + const first = http.request({ + host: '127.0.0.1', port: server.port, path: '/held/foo.pdb/abc/foo.pdb', method: 'GET', + }); + first.on('error', () => {}); + first.end(); + while (upstream.requests.length === 0) await sleep(10); + first.destroy(); + + // The upstream request must actually be canceled, not left running. + const abortDeadline = Date.now() + 2000; + while (aborts === 0 && Date.now() < abortDeadline) await sleep(10); + assert.equal(aborts, 1, 'upstream must observe the abort after the client disconnects'); + assert.equal(active, 0, 'upstream must have no active request left'); + await sleep(50); // let the freed slot settle server-side + + // A distinct second request may now use the freed slot — but must never + // have overlapped with the first at upstream. + const second = request(server.port, '/other/bar.pdb/def/bar.pdb'); + const reachDeadline = Date.now() + 2000; + while (upstream.requests.length < 2 && Date.now() < reachDeadline) await sleep(10); + assert.equal(upstream.requests.length, 2, 'second request should reach upstream after the abort'); + release(); + const res2 = await second; + assert.equal(res2.statusCode, 200); + assert.ok(maxActive <= 1, `upstream must never see 2 simultaneous active requests, saw ${maxActive}`); +}); + test('concurrent requests for the same missing path only hit upstream once', async (t) => { const { server, upstream } = await startProxy(t, { handler: (req, res) => { From c3e08a2610c715b62ad2b8f260a71085d133c444 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 15:58:06 +0000 Subject: [PATCH 5/9] Add a sampled user-agent request log Logging every request would be too noisy at ~13M requests/day, so emit one greppable request-sample line for roughly 1 in 1000 requests (tunable via UA_LOG_SAMPLE_RATE) with the method, path, response disposition, and User-Agent. Sampling uses a deterministic modulo counter so the cadence is testable. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01ToXSyZGzHfwtNoF6mWoJUA --- src/index.ts | 28 +++++++++++++++++++++++++++- test/helpers.js | 6 +++++- test/server.test.js | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 2 deletions(-) diff --git a/src/index.ts b/src/index.ts index 9137f83..2e9aee0 100644 --- a/src/index.ts +++ b/src/index.ts @@ -5,7 +5,7 @@ import httpProxy from 'http-proxy'; import LRU from 'lru-cache'; import * as url from 'url'; -const { PATH_PREFIX, TARGET_HOST, MAX_UPSTREAM_CONCURRENCY } = process.env; +const { PATH_PREFIX, TARGET_HOST, MAX_UPSTREAM_CONCURRENCY, UA_LOG_SAMPLE_RATE } = process.env; assert(TARGET_HOST, 'TARGET_HOST is defined'); @@ -24,6 +24,13 @@ const HIT_CACHE_CONTROL = 'public, max-age=604800, immutable'; const UPSTREAM_CONCURRENCY_LIMIT = parseInt(MAX_UPSTREAM_CONCURRENCY || '', 10) || 100; const RETRY_AFTER_SECONDS = 30; +// Log roughly 1 in N requests. The service fields ~13M requests/day, so +// logging every one would swamp the log drain; a sampled line is enough to +// see which user agents are hitting us and how their requests are answered. +// Sampling uses a modulo counter rather than Math.random so the cadence is +// deterministic and testable. +const REQUEST_LOG_SAMPLE_RATE = parseInt(UA_LOG_SAMPLE_RATE || '', 10) || 1000; + const TARGET_URL = url.format({ protocol: 'https:', slashes: true, @@ -186,6 +193,20 @@ proxy.on('error', (err, req, res) => { res.end(`Something went wrong. If this happens consistently please report to https://github.com/electron/symbol-server with this error ID: "${errorId}"`); }); +// Called once per request at the point where its disposition (redirect, +// cached-404, shed, proxied) becomes cheaply known; every +// REQUEST_LOG_SAMPLE_RATE-th call emits a single greppable line. The +// User-Agent is stripped of newlines and has quotes escaped so the ua="..." +// field always stays on one line and cannot be broken out of. +let sampledRequestCount = 0; + +function sampleRequestLog(req: http.IncomingMessage, disposition: string) { + if (sampledRequestCount++ % REQUEST_LOG_SAMPLE_RATE !== 0) return; + const userAgent = req.headers['user-agent']; + const ua = userAgent ? userAgent.replace(/[\r\n]+/g, ' ').replace(/"/g, '\\"') : '-'; + console.log(`request-sample method=${req.method} path=${req.url} disposition=${disposition} ua="${ua}"`); +} + http.createServer((req, res) => { const parsed = new url.URL(`http://localhost${req.url!}`); if (parsed.pathname === '/health') { @@ -197,6 +218,7 @@ http.createServer((req, res) => { const isSentryRequest = userAgent && userAgent.startsWith('symbolicator/'); if (isSentryRequest || req.headers['x-electron-symbol-redirect'] === '1') { + sampleRequestLog(req, 'redirect'); res.setHeader('Location', url.format({ protocol: 'https:', slashes: true, @@ -208,6 +230,7 @@ http.createServer((req, res) => { } if (missingSymbolCache.get(cacheKey)) { + sampleRequestLog(req, 'cached-404'); return res.writeHead(404, { 'Cache-Control': MISSING_CACHE_CONTROL }).end(); } @@ -227,6 +250,7 @@ http.createServer((req, res) => { // nothing to clean up. if (clientGone(req, res)) return; if (missingSymbolCache.get(cacheKey)) { + sampleRequestLog(req, 'cached-404'); return res.writeHead(404, { 'Cache-Control': MISSING_CACHE_CONTROL }).end(); } proxyToUpstream(req, res, cacheKey); @@ -259,6 +283,7 @@ function proxyToUpstream(req: http.IncomingMessage, res: http.ServerResponse, ca if (activeUpstreamRequests >= UPSTREAM_CONCURRENCY_LIMIT) { // Shed load immediately instead of queueing behind a saturated upstream, // otherwise the router backlog fills up and everyone gets H11 503s. + sampleRequestLog(req, 'shed'); res.setHeader('Retry-After', String(RETRY_AFTER_SECONDS)); return res.writeHead(503).end('Too many concurrent symbol requests, retry later'); } @@ -332,6 +357,7 @@ function proxyToUpstream(req: http.IncomingMessage, res: http.ServerResponse, ca return; } + sampleRequestLog(req, 'proxied'); proxy.web(req, res, { target: TARGET_URL }); } diff --git a/test/helpers.js b/test/helpers.js index 17939a5..a660027 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -97,8 +97,9 @@ async function startSymbolServer({ targetHost, pathPrefix, env: extraEnv } = {}) else delete env.PATH_PREFIX; const stderrChunks = []; + const stdoutChunks = []; const child = spawn(process.execPath, [SERVER_ENTRY], { env }); - child.stdout.on('data', () => {}); + child.stdout.on('data', (d) => stdoutChunks.push(d)); child.stderr.on('data', (d) => stderrChunks.push(d)); let exited = false; @@ -124,6 +125,9 @@ async function startSymbolServer({ targetHost, pathPrefix, env: extraEnv } = {}) return { port, + // Everything the server has written to stdout so far (e.g. sampled + // request logs), as a string. + stdout: () => Buffer.concat(stdoutChunks).toString(), stop: () => new Promise((resolve) => { if (exited) return resolve(); diff --git a/test/server.test.js b/test/server.test.js index 94f7367..bc187ce 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -444,6 +444,40 @@ test('concurrent requests for the same missing path only hit upstream once', asy assert.equal(upstream.requests.length, 1, 'duplicate lookup should not reach upstream'); }); +test('sampled request log fires at the configured cadence and includes the user-agent', async (t) => { + const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); + const { server } = await startProxy(t, { + env: { UA_LOG_SAMPLE_RATE: '5' }, + handler: (req, res) => { + res.writeHead(200); + res.end('ok'); + }, + }); + + const UA = 'symsrv-test/2.0 ("Windows")'; + for (let i = 0; i < 12; i++) { + const res = await request(server.port, `/sampled/foo.pdb/abc/foo-${i}.pdb`, { + 'user-agent': UA, + }); + assert.equal(res.statusCode, 200); + } + + // The modulo counter samples requests 1, 6 and 11 of the 12. Poll briefly: + // the child's stdout pipe delivers asynchronously. + let lines; + const deadline = Date.now() + 2000; + do { + lines = server.stdout().split('\n').filter((l) => l.startsWith('request-sample ')); + if (lines.length >= 3) break; + await sleep(20); + } while (Date.now() < deadline); + + assert.equal(lines.length, 3, `expected 3 sampled lines for 12 requests at 1/5, got:\n${server.stdout()}`); + for (const line of lines) { + assert.match(line, /^request-sample method=GET path=\/sampled\/foo\.pdb\/abc\/foo-\d+\.pdb disposition=proxied ua="symsrv-test\/2\.0 \(\\"Windows\\"\)"$/); + } +}); + test('proxy returns 500 with error ID when upstream is unreachable', async (t) => { const server = await startSymbolServer({ targetHost: '127.0.0.1:1' }); t.after(() => server.stop()); From 8eeac33d99c6bff452772d31b2306c4bf790a08b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 16:47:18 +0000 Subject: [PATCH 6/9] Make 302 redirect responses edge-cacheable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cloudflare now caches symbolicator 302s at the edge (cache key includes a per-UA-cohort marker), so the redirect path must send a cacheable Cache-Control. Reuse MISSING_CACHE_CONTROL (public, max-age=3600) — the Location depends only on the request path, so caching is safe. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01ToXSyZGzHfwtNoF6mWoJUA --- src/index.ts | 3 ++- test/server.test.js | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/index.ts b/src/index.ts index 2e9aee0..6a4bc76 100644 --- a/src/index.ts +++ b/src/index.ts @@ -225,7 +225,8 @@ http.createServer((req, res) => { host: TARGET_HOST, pathname: cacheKey, })); - res.setHeader('Cache-Control', 'no-store'); + // Cloudflare caches these 302s at the edge per UA cohort; Location depends only on the path. + res.setHeader('Cache-Control', MISSING_CACHE_CONTROL); return res.writeHead(302).end(); } diff --git a/test/server.test.js b/test/server.test.js index bc187ce..0ab7851 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -222,7 +222,7 @@ test('upstream Cache-Control on 200s is preserved', async (t) => { assert.equal(res.headers['cache-control'], 'public, max-age=60'); }); -test('redirect responses are not cacheable', async (t) => { +test('redirect responses are edge-cacheable', async (t) => { const server = await startSymbolServer({ targetHost: 'symbols.example.test' }); t.after(() => server.stop()); @@ -230,7 +230,7 @@ test('redirect responses are not cacheable', async (t) => { 'user-agent': 'symbolicator/1.2.3', }); assert.equal(res.statusCode, 302); - assert.equal(res.headers['cache-control'], 'no-store'); + assert.equal(res.headers['cache-control'], 'public, max-age=3600'); }); test('sheds load with 503 + Retry-After above the upstream concurrency cap', async (t) => { From cc3b04487ca85ebcc1b47d70e1fb0dcef4e688aa Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 16:51:33 +0000 Subject: [PATCH 7/9] Revert "Add a sampled user-agent request log" This reverts commit c3e08a2610c715b62ad2b8f260a71085d133c444. --- src/index.ts | 28 +--------------------------- test/helpers.js | 6 +----- test/server.test.js | 34 ---------------------------------- 3 files changed, 2 insertions(+), 66 deletions(-) diff --git a/src/index.ts b/src/index.ts index 6a4bc76..d774011 100644 --- a/src/index.ts +++ b/src/index.ts @@ -5,7 +5,7 @@ import httpProxy from 'http-proxy'; import LRU from 'lru-cache'; import * as url from 'url'; -const { PATH_PREFIX, TARGET_HOST, MAX_UPSTREAM_CONCURRENCY, UA_LOG_SAMPLE_RATE } = process.env; +const { PATH_PREFIX, TARGET_HOST, MAX_UPSTREAM_CONCURRENCY } = process.env; assert(TARGET_HOST, 'TARGET_HOST is defined'); @@ -24,13 +24,6 @@ const HIT_CACHE_CONTROL = 'public, max-age=604800, immutable'; const UPSTREAM_CONCURRENCY_LIMIT = parseInt(MAX_UPSTREAM_CONCURRENCY || '', 10) || 100; const RETRY_AFTER_SECONDS = 30; -// Log roughly 1 in N requests. The service fields ~13M requests/day, so -// logging every one would swamp the log drain; a sampled line is enough to -// see which user agents are hitting us and how their requests are answered. -// Sampling uses a modulo counter rather than Math.random so the cadence is -// deterministic and testable. -const REQUEST_LOG_SAMPLE_RATE = parseInt(UA_LOG_SAMPLE_RATE || '', 10) || 1000; - const TARGET_URL = url.format({ protocol: 'https:', slashes: true, @@ -193,20 +186,6 @@ proxy.on('error', (err, req, res) => { res.end(`Something went wrong. If this happens consistently please report to https://github.com/electron/symbol-server with this error ID: "${errorId}"`); }); -// Called once per request at the point where its disposition (redirect, -// cached-404, shed, proxied) becomes cheaply known; every -// REQUEST_LOG_SAMPLE_RATE-th call emits a single greppable line. The -// User-Agent is stripped of newlines and has quotes escaped so the ua="..." -// field always stays on one line and cannot be broken out of. -let sampledRequestCount = 0; - -function sampleRequestLog(req: http.IncomingMessage, disposition: string) { - if (sampledRequestCount++ % REQUEST_LOG_SAMPLE_RATE !== 0) return; - const userAgent = req.headers['user-agent']; - const ua = userAgent ? userAgent.replace(/[\r\n]+/g, ' ').replace(/"/g, '\\"') : '-'; - console.log(`request-sample method=${req.method} path=${req.url} disposition=${disposition} ua="${ua}"`); -} - http.createServer((req, res) => { const parsed = new url.URL(`http://localhost${req.url!}`); if (parsed.pathname === '/health') { @@ -218,7 +197,6 @@ http.createServer((req, res) => { const isSentryRequest = userAgent && userAgent.startsWith('symbolicator/'); if (isSentryRequest || req.headers['x-electron-symbol-redirect'] === '1') { - sampleRequestLog(req, 'redirect'); res.setHeader('Location', url.format({ protocol: 'https:', slashes: true, @@ -231,7 +209,6 @@ http.createServer((req, res) => { } if (missingSymbolCache.get(cacheKey)) { - sampleRequestLog(req, 'cached-404'); return res.writeHead(404, { 'Cache-Control': MISSING_CACHE_CONTROL }).end(); } @@ -251,7 +228,6 @@ http.createServer((req, res) => { // nothing to clean up. if (clientGone(req, res)) return; if (missingSymbolCache.get(cacheKey)) { - sampleRequestLog(req, 'cached-404'); return res.writeHead(404, { 'Cache-Control': MISSING_CACHE_CONTROL }).end(); } proxyToUpstream(req, res, cacheKey); @@ -284,7 +260,6 @@ function proxyToUpstream(req: http.IncomingMessage, res: http.ServerResponse, ca if (activeUpstreamRequests >= UPSTREAM_CONCURRENCY_LIMIT) { // Shed load immediately instead of queueing behind a saturated upstream, // otherwise the router backlog fills up and everyone gets H11 503s. - sampleRequestLog(req, 'shed'); res.setHeader('Retry-After', String(RETRY_AFTER_SECONDS)); return res.writeHead(503).end('Too many concurrent symbol requests, retry later'); } @@ -358,7 +333,6 @@ function proxyToUpstream(req: http.IncomingMessage, res: http.ServerResponse, ca return; } - sampleRequestLog(req, 'proxied'); proxy.web(req, res, { target: TARGET_URL }); } diff --git a/test/helpers.js b/test/helpers.js index a660027..17939a5 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -97,9 +97,8 @@ async function startSymbolServer({ targetHost, pathPrefix, env: extraEnv } = {}) else delete env.PATH_PREFIX; const stderrChunks = []; - const stdoutChunks = []; const child = spawn(process.execPath, [SERVER_ENTRY], { env }); - child.stdout.on('data', (d) => stdoutChunks.push(d)); + child.stdout.on('data', () => {}); child.stderr.on('data', (d) => stderrChunks.push(d)); let exited = false; @@ -125,9 +124,6 @@ async function startSymbolServer({ targetHost, pathPrefix, env: extraEnv } = {}) return { port, - // Everything the server has written to stdout so far (e.g. sampled - // request logs), as a string. - stdout: () => Buffer.concat(stdoutChunks).toString(), stop: () => new Promise((resolve) => { if (exited) return resolve(); diff --git a/test/server.test.js b/test/server.test.js index 0ab7851..7e51502 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -444,40 +444,6 @@ test('concurrent requests for the same missing path only hit upstream once', asy assert.equal(upstream.requests.length, 1, 'duplicate lookup should not reach upstream'); }); -test('sampled request log fires at the configured cadence and includes the user-agent', async (t) => { - const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); - const { server } = await startProxy(t, { - env: { UA_LOG_SAMPLE_RATE: '5' }, - handler: (req, res) => { - res.writeHead(200); - res.end('ok'); - }, - }); - - const UA = 'symsrv-test/2.0 ("Windows")'; - for (let i = 0; i < 12; i++) { - const res = await request(server.port, `/sampled/foo.pdb/abc/foo-${i}.pdb`, { - 'user-agent': UA, - }); - assert.equal(res.statusCode, 200); - } - - // The modulo counter samples requests 1, 6 and 11 of the 12. Poll briefly: - // the child's stdout pipe delivers asynchronously. - let lines; - const deadline = Date.now() + 2000; - do { - lines = server.stdout().split('\n').filter((l) => l.startsWith('request-sample ')); - if (lines.length >= 3) break; - await sleep(20); - } while (Date.now() < deadline); - - assert.equal(lines.length, 3, `expected 3 sampled lines for 12 requests at 1/5, got:\n${server.stdout()}`); - for (const line of lines) { - assert.match(line, /^request-sample method=GET path=\/sampled\/foo\.pdb\/abc\/foo-\d+\.pdb disposition=proxied ua="symsrv-test\/2\.0 \(\\"Windows\\"\)"$/); - } -}); - test('proxy returns 500 with error ID when upstream is unreachable', async (t) => { const server = await startSymbolServer({ targetHost: '127.0.0.1:1' }); t.after(() => server.stop()); From 9f57a4e3526b806698c874ee067445dde06a440c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 16:56:12 +0000 Subject: [PATCH 8/9] Slim to cache headers + bigger negative cache; drop request tracking now that Cloudflare fronts the origin Cloudflare now fronts the origin with a WAF, a per-IP rate limit, and edge caching, so the app-side upstream concurrency cap, in-flight request dedup, and client-disconnect cancellation are redundant. Keep the byte-bounded misses-only negative cache, the Cache-Control headers that make responses edge-cacheable, and the proxy error-handler guard against writing to an already-closed response. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01ToXSyZGzHfwtNoF6mWoJUA --- src/index.ts | 186 ++------------------------------------ test/helpers.js | 10 +-- test/server.test.js | 212 -------------------------------------------- 3 files changed, 8 insertions(+), 400 deletions(-) diff --git a/src/index.ts b/src/index.ts index d774011..1689ea5 100644 --- a/src/index.ts +++ b/src/index.ts @@ -5,7 +5,7 @@ import httpProxy from 'http-proxy'; import LRU from 'lru-cache'; import * as url from 'url'; -const { PATH_PREFIX, TARGET_HOST, MAX_UPSTREAM_CONCURRENCY } = process.env; +const { PATH_PREFIX, TARGET_HOST } = process.env; assert(TARGET_HOST, 'TARGET_HOST is defined'); @@ -18,12 +18,6 @@ const MISSING_CACHE_CONTROL = `public, max-age=${MISSING_SYMBOL_TTL_SECONDS}`; // aggressively by CDNs/clients. const HIT_CACHE_CONTROL = 'public, max-age=604800, immutable'; -// Cap on concurrent proxied upstream requests. Beyond this we shed load -// immediately with a 503 instead of queueing, so the dyno's backlog stays -// shallow during floods. -const UPSTREAM_CONCURRENCY_LIMIT = parseInt(MAX_UPSTREAM_CONCURRENCY || '', 10) || 100; -const RETRY_AFTER_SECONDS = 30; - const TARGET_URL = url.format({ protocol: 'https:', slashes: true, @@ -70,41 +64,6 @@ const missingSymbolCache = new LRU({ maxAge: MISSING_SYMBOL_TTL_SECONDS * 1000, }); -// Proxied requests currently awaiting an upstream response, keyed by rewritten -// path. Used only to dedupe identical concurrent lookups — NOT for the -// concurrency cap: multiple proxied requests for the same path share a single -// map entry, so Map.size undercounts. -const inFlightRequests = new Map>(); - -// Number of proxied upstream requests actually in flight right now. This is -// what the concurrency cap is enforced against. -let activeUpstreamRequests = 0; - -// Ties each proxied downstream request to the upstream request http-proxy -// opens for it. The shared 'proxyReq' hook below fires for every proxied -// request, so it must map each proxyReq back to the right downstream request; -// keying by the incoming request object does that without any cleanup -// bookkeeping (entries die with the request). -interface UpstreamLifecycle { - proxyReq: http.ClientRequest | null; - downstreamGone: boolean; - settle: () => void; -} - -const upstreamLifecycles = new WeakMap(); - -// Cancel the outgoing upstream request. destroy() on a request that already -// completed just tears down its (connection: close) socket, but be defensive: -// a throw here would bubble into an event handler and kill nothing gracefully. -function abortUpstreamRequest(proxyReq: http.ClientRequest) { - if (proxyReq.destroyed) return; - try { - proxyReq.destroy(); - } catch (err) { - console.error('Failed to abort upstream request:', err); - } -} - function incomingPathToProxyPath(path: string): string { // symstore.exe and symsrv.dll don't always agree on the case of the path to a // given symbol file. Since our artifact URLs are case-sensitive, this causes symbol @@ -153,20 +112,6 @@ proxy.on('proxyReq', (proxyReq, request, response, options) => { } return originalWriteHead.apply(response, args); }; - - const lifecycle = upstreamLifecycles.get(request); - if (lifecycle) { - lifecycle.proxyReq = proxyReq; - // The upstream slot and dedup promise settle on the UPSTREAM request's - // lifecycle, not the downstream response's: 'close' fires both when the - // proxied response has been fully read and when the request is destroyed - // or errors, so normal completion and client-abort cancellation route to - // the same idempotent settle. - proxyReq.on('close', lifecycle.settle); - // The client may have vanished between proxy.web() and the socket - // assignment that fires this event; cancel the upstream work right away. - if (lifecycle.downstreamGone) abortUpstreamRequest(proxyReq); - } }); proxy.on('error', (err, req, res) => { @@ -174,9 +119,9 @@ proxy.on('error', (err, req, res) => { console.error('Error:', errorId, 'Request:', req.url, err); - // A deliberately canceled upstream request (client disconnected mid-proxy) - // can surface its teardown error here; there is no one left to answer and - // writing headers to a closed/finished response would throw. + // The client may already be gone (disconnected mid-proxy) by the time the + // upstream request fails; writing headers to a closed/finished response + // would throw. if (res.destroyed || res.writableEnded || res.headersSent) return; res.writeHead(500, { @@ -212,129 +157,8 @@ http.createServer((req, res) => { return res.writeHead(404, { 'Cache-Control': MISSING_CACHE_CONTROL }).end(); } - // If an identical lookup is already being proxied, wait for it to settle - // rather than launching a duplicate upstream fetch. If it negative-cached - // the path we can answer 404 for free, otherwise proxy as usual — - // proxyToUpstream re-checks the concurrency cap when we wake, so a stampede - // of same-path waiters is shed instead of all proxying at once. - const inFlight = inFlightRequests.get(cacheKey); - if (inFlight) { - inFlight.then(() => { - // The client may have hung up while we waited on the leader. Its - // response 'close' event has already fired by now, so proxying would - // register settle listeners that never run and leak an upstream slot - // forever (proxyToUpstream re-checks this, but bail early and skip the - // cache lookup too). Dropped waiters touch no counters, so there is - // nothing to clean up. - if (clientGone(req, res)) return; - if (missingSymbolCache.get(cacheKey)) { - return res.writeHead(404, { 'Cache-Control': MISSING_CACHE_CONTROL }).end(); - } - proxyToUpstream(req, res, cacheKey); - }); - return; - } - - proxyToUpstream(req, res, cacheKey); -}).listen(process.env.PORT || 8080); - -// True when the client that issued this request can no longer receive a -// response: its socket is gone (disconnect — note 'close' fires on the -// response even before headers are written) or the response already ended. -function clientGone(req: http.IncomingMessage, res: http.ServerResponse): boolean { - return ( - req.destroyed || - res.destroyed || - res.writableEnded || - !res.socket || - res.socket.destroyed - ); -} - -function proxyToUpstream(req: http.IncomingMessage, res: http.ServerResponse, cacheKey: string) { - // Never proxy on behalf of a client that already disconnected: its 'close' - // event has already fired, so the settle listeners below would never run - // and the upstream slot would leak until process restart. - if (clientGone(req, res)) return; - - if (activeUpstreamRequests >= UPSTREAM_CONCURRENCY_LIMIT) { - // Shed load immediately instead of queueing behind a saturated upstream, - // otherwise the router backlog fills up and everyone gets H11 503s. - res.setHeader('Retry-After', String(RETRY_AFTER_SECONDS)); - return res.writeHead(503).end('Too many concurrent symbol requests, retry later'); - } - - activeUpstreamRequests++; - // settle() can be reached from several events (the upstream request's - // 'close', the downstream 'close'/'error' fallback below, and by hand when - // the client disconnected before the listeners were registered); settle - // exactly once so the active count can never be decremented twice. - let settled = false; - let settle!: () => void; - const inFlight = new Promise((resolve) => { - settle = () => { - if (settled) return; - settled = true; - activeUpstreamRequests--; - // Several proxied requests for the same path can coexist (dedup waiters - // that woke below the cap); only the one registered in the map may - // remove the entry, or a later request's dedup entry would be dropped - // while it is still in flight. - if (inFlightRequests.get(cacheKey) === inFlight) { - inFlightRequests.delete(cacheKey); - } - resolve(); - }; - }); - - // Tie teardown to the actual upstream request rather than the downstream - // response alone: http-proxy does not cancel the outgoing request by itself - // when the client disconnects mid-proxy (its req 'aborted' hook never fires - // on modern Node for requests whose body was already fully received, i.e. - // every GET), so settling on downstream 'close' freed the slot and woke - // dedup waiters while the upstream fetch was still running — bypassing the - // cap. Instead, downstream 'close'/'error' destroys the upstream request, - // and the slot/dedup promise settle only once that request has ended or - // been aborted (its 'close' listener, registered in the proxyReq hook), so - // waiters can never wake into a still-occupied slot. - const lifecycle: UpstreamLifecycle = { proxyReq: null, downstreamGone: false, settle }; - upstreamLifecycles.set(req, lifecycle); - const onDownstreamGone = () => { - if (lifecycle.downstreamGone) return; - lifecycle.downstreamGone = true; - if (lifecycle.proxyReq) { - // Cancel the upstream work; settle fires when the destroyed request - // emits 'close'. (After a normal completion this destroy is a no-op on - // an already-finished request.) - abortUpstreamRequest(lifecycle.proxyReq); - } else { - // No upstream request was captured for this response — either we never - // reached proxy.web below, or http-proxy skipped the proxyReq event - // (it does for Expect: 100-continue requests). Nothing to cancel; - // settle now so the slot cannot leak. Should the capture still happen a - // tick later, the downstreamGone flag above makes it destroy the - // upstream request immediately. - settle(); - } - }; - res.on('close', onDownstreamGone); - res.on('error', onDownstreamGone); - - if (!inFlightRequests.has(cacheKey)) { - inFlightRequests.set(cacheKey, inFlight); - } - - // 'close' fires at most once. If the client vanished between the clientGone - // check at the top of this function and the listener registration above, it - // has already fired and never will again — settle by hand and skip the - // upstream fetch entirely. - if (clientGone(req, res)) { - settle(); - return; - } - proxy.web(req, res, { target: TARGET_URL }); -} +}).listen(process.env.PORT || 8080); process.on('uncaughtException', (err) => { // Avoid process dieing on uncaughtException diff --git a/test/helpers.js b/test/helpers.js index 17939a5..eea6b85 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -70,9 +70,6 @@ function startUpstream(handler) { requests, close: () => new Promise((res) => { - // Sever any connections still open (e.g. leaked by a bug under - // test) so close() cannot hang the test runner's after-hooks. - server.closeAllConnections(); server.close(() => res()); }), }); @@ -80,12 +77,11 @@ function startUpstream(handler) { }); } -async function startSymbolServer({ targetHost, pathPrefix, env: extraEnv } = {}) { +async function startSymbolServer({ targetHost, pathPrefix } = {}) { const port = await getFreePort(); const env = { ...process.env, - ...extraEnv, TARGET_HOST: targetHost, PORT: String(port), // http-proxy uses the default https agent; NODE_EXTRA_CA_CERTS is the @@ -138,14 +134,14 @@ async function startSymbolServer({ targetHost, pathPrefix, env: extraEnv } = {}) // Spawn an upstream + symbol-server pair and register cleanup with the test // context. Returns { server, upstream }. -async function startProxy(t, { handler, pathPrefix, env } = {}) { +async function startProxy(t, { handler, pathPrefix } = {}) { const upstream = await startUpstream(handler || ((req, res) => { res.writeHead(200); res.end('ok'); })); t.after(() => upstream.close()); - const server = await startSymbolServer({ targetHost: upstream.host, pathPrefix, env }); + const server = await startSymbolServer({ targetHost: upstream.host, pathPrefix }); t.after(() => server.stop()); return { server, upstream }; diff --git a/test/server.test.js b/test/server.test.js index 7e51502..cf1b047 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -1,6 +1,5 @@ 'use strict'; -const http = require('node:http'); const test = require('node:test'); const assert = require('node:assert/strict'); @@ -233,217 +232,6 @@ test('redirect responses are edge-cacheable', async (t) => { assert.equal(res.headers['cache-control'], 'public, max-age=3600'); }); -test('sheds load with 503 + Retry-After above the upstream concurrency cap', async (t) => { - let releaseFirst; - const firstHeld = new Promise((resolve) => { releaseFirst = resolve; }); - const { server, upstream } = await startProxy(t, { - env: { MAX_UPSTREAM_CONCURRENCY: '1' }, - handler: async (req, res) => { - await firstHeld; - res.writeHead(200); - res.end('ok'); - }, - }); - - const first = request(server.port, '/held/foo.pdb/abc/foo.pdb'); - // Wait until the first request has actually reached upstream. - while (upstream.requests.length === 0) { - await new Promise((resolve) => setTimeout(resolve, 10)); - } - - const shed = await request(server.port, '/other/foo.pdb/abc/foo.pdb'); - assert.equal(shed.statusCode, 503); - assert.equal(shed.headers['retry-after'], '30'); - assert.equal(upstream.requests.length, 1, 'shed request should not reach upstream'); - - releaseFirst(); - const held = await first; - assert.equal(held.statusCode, 200); -}); - -test('same-path dedup waiters cannot bypass the upstream concurrency cap', async (t) => { - // Regression test: waiters queued behind an in-flight leader used to all - // call proxyToUpstream when the leader settled. Each overwrote the same - // in-flight map key, so the Map.size-based cap check saw 1 while N upstream - // requests were actually active (observed: 6 with a cap of 2). - let active = 0; - let maxActive = 0; - let phase = 'leader'; - let releaseLeader; - const leaderHeld = new Promise((resolve) => { releaseLeader = resolve; }); - let releaseWaiters; - const waitersHeld = new Promise((resolve) => { releaseWaiters = resolve; }); - - const { server, upstream } = await startProxy(t, { - env: { MAX_UPSTREAM_CONCURRENCY: '2' }, - handler: async (req, res) => { - active += 1; - maxActive = Math.max(maxActive, active); - if (phase === 'leader') await leaderHeld; else await waitersHeld; - res.writeHead(200); - res.end('ok'); - active -= 1; - }, - }); - - const PATH = '/stampede/foo.pdb/abc/foo.pdb'; - const leader = request(server.port, PATH); - while (upstream.requests.length === 0) { - await new Promise((resolve) => setTimeout(resolve, 10)); - } - - // Queue six identical lookups; all should dedup-wait on the leader. - phase = 'waiters'; - const waiters = []; - for (let i = 0; i < 6; i++) waiters.push(request(server.port, PATH)); - await new Promise((resolve) => setTimeout(resolve, 200)); - assert.equal(upstream.requests.length, 1, 'waiters must not reach upstream while leader is in flight'); - - // Leader succeeds; woken waiters re-check the cap, so only two may proxy. - releaseLeader(); - const deadline = Date.now() + 2000; - while (upstream.requests.length < 3 && Date.now() < deadline) { - await new Promise((resolve) => setTimeout(resolve, 10)); - } - // Grace period to catch any waiters that slipped past the cap. - await new Promise((resolve) => setTimeout(resolve, 200)); - releaseWaiters(); - - const leaderRes = await leader; - const waiterRes = await Promise.all(waiters); - - assert.equal(leaderRes.statusCode, 200); - assert.ok(maxActive <= 2, `at most 2 simultaneous upstream requests allowed, saw ${maxActive}`); - assert.equal(upstream.requests.length, 3, 'leader + at most cap-many waiters may reach upstream'); - - const okCount = waiterRes.filter((r) => r.statusCode === 200).length; - const shed = waiterRes.filter((r) => r.statusCode === 503); - assert.equal(okCount, 2, 'exactly cap-many waiters should be proxied'); - assert.equal(shed.length, 4, 'remaining waiters should be shed'); - for (const r of shed) assert.equal(r.headers['retry-after'], '30'); -}); - -test('a dedup waiter whose client disconnects mid-wait does not leak an upstream slot', async (t) => { - // Regression test: a same-path waiter used to call proxyToUpstream when the - // leader settled even if its own client had already hung up. The response's - // 'close' event had fired before the settle listeners were registered, so - // settle never ran and the incremented activeUpstreamRequests slot leaked - // forever. With a cap of 1 a single canceled waiter then turned every - // subsequent distinct-path request into a 503 until process restart. - const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); - let phase = 'leader'; - let releaseLeader; - const leaderHeld = new Promise((resolve) => { releaseLeader = resolve; }); - const { server, upstream } = await startProxy(t, { - env: { MAX_UPSTREAM_CONCURRENCY: '1' }, - handler: async (req, res) => { - if (phase === 'leader') await leaderHeld; - res.writeHead(200); - res.end('ok'); - }, - }); - - const PATH = '/held/foo.pdb/abc/foo.pdb'; - const leader = request(server.port, PATH); - while (upstream.requests.length === 0) await sleep(10); - - // Same-path waiter; destroy its client socket while it waits on the leader. - const waiter = http.request({ host: '127.0.0.1', port: server.port, path: PATH, method: 'GET' }); - waiter.on('error', () => {}); - waiter.end(); - await sleep(200); // let the server register it as a dedup waiter - waiter.destroy(); - await sleep(200); // let the server-side 'close' fire - - phase = 'done'; - releaseLeader(); - const leaderRes = await leader; - assert.equal(leaderRes.statusCode, 200); - await sleep(200); // let the canceled waiter wake and (previously) leak - - const probe = await request(server.port, '/distinct/bar.pdb/def/bar.pdb'); - assert.equal(probe.statusCode, 200, 'canceled waiter must not leak an upstream slot'); - assert.equal(upstream.requests.length, 2, 'only the leader and the probe should reach upstream'); -}); - -test('client disconnect mid-proxy aborts upstream and frees the slot only after', async (t) => { - // Regression test: the slot counter and dedup promise used to settle when - // the DOWNSTREAM response closed, but http-proxy does not cancel the - // UPSTREAM request on its own (its req 'aborted' hook never fires for - // fully-received requests on modern Node). A client disconnecting mid-proxy - // therefore freed its slot while the upstream fetch kept running: with a - // cap of 1, a distinct second request then also reached upstream, which saw - // 2 simultaneous active requests and never observed an abort. - const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); - let active = 0; - let maxActive = 0; - let aborts = 0; - let release; - const held = new Promise((resolve) => { release = resolve; }); - const { server, upstream } = await startProxy(t, { - env: { MAX_UPSTREAM_CONCURRENCY: '1' }, - handler: async (req, res) => { - active += 1; - maxActive = Math.max(maxActive, active); - res.on('close', () => { - if (!res.writableEnded) aborts += 1; - active -= 1; - }); - await held; - if (!res.destroyed) { - res.writeHead(200); - res.end('ok'); - } - }, - }); - - // First request reaches the held upstream, then its client disconnects. - const first = http.request({ - host: '127.0.0.1', port: server.port, path: '/held/foo.pdb/abc/foo.pdb', method: 'GET', - }); - first.on('error', () => {}); - first.end(); - while (upstream.requests.length === 0) await sleep(10); - first.destroy(); - - // The upstream request must actually be canceled, not left running. - const abortDeadline = Date.now() + 2000; - while (aborts === 0 && Date.now() < abortDeadline) await sleep(10); - assert.equal(aborts, 1, 'upstream must observe the abort after the client disconnects'); - assert.equal(active, 0, 'upstream must have no active request left'); - await sleep(50); // let the freed slot settle server-side - - // A distinct second request may now use the freed slot — but must never - // have overlapped with the first at upstream. - const second = request(server.port, '/other/bar.pdb/def/bar.pdb'); - const reachDeadline = Date.now() + 2000; - while (upstream.requests.length < 2 && Date.now() < reachDeadline) await sleep(10); - assert.equal(upstream.requests.length, 2, 'second request should reach upstream after the abort'); - release(); - const res2 = await second; - assert.equal(res2.statusCode, 200); - assert.ok(maxActive <= 1, `upstream must never see 2 simultaneous active requests, saw ${maxActive}`); -}); - -test('concurrent requests for the same missing path only hit upstream once', async (t) => { - const { server, upstream } = await startProxy(t, { - handler: (req, res) => { - setTimeout(() => { - res.writeHead(403); - res.end(); - }, 100); - }, - }); - - const [first, second] = await Promise.all([ - request(server.port, '/dup/foo.pdb/abc/foo.pdb'), - request(server.port, '/dup/foo.pdb/abc/foo.pdb'), - ]); - assert.equal(first.statusCode, 404); - assert.equal(second.statusCode, 404); - assert.equal(upstream.requests.length, 1, 'duplicate lookup should not reach upstream'); -}); - test('proxy returns 500 with error ID when upstream is unreachable', async (t) => { const server = await startSymbolServer({ targetHost: '127.0.0.1:1' }); t.after(() => server.stop()); From 9c6ed74fbfbb8e3911eb3b68c9a2d12de876d6f1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 17:08:06 +0000 Subject: [PATCH 9/9] Restrict 302 caching to Cloudflare's cohort-aware cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The redirect branch triggers on either the symbolicator/ user-agent or the x-electron-symbol-redirect header, but only the UA marker is in Cloudflare's cache key — and the 302 carried a public Cache-Control with no Vary, so an ordinary-UA request with the redirect header could cache a redirect where generic caches (or a cohort-unaware window at the edge) would serve it to normal clients. Send Cache-Control: no-store so browsers and generic shared caches never store the redirect, plus Cloudflare-CDN-Cache-Control: public, max-age=3600 — a Cloudflare-specific directive that Cloudflare prefers over Cache-Control and does not forward downstream — so the one cache with the cohort-aware key still caches it for an hour. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01ToXSyZGzHfwtNoF6mWoJUA --- src/index.ts | 11 +++++++++-- test/server.test.js | 7 +++++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/index.ts b/src/index.ts index 1689ea5..b2d00d9 100644 --- a/src/index.ts +++ b/src/index.ts @@ -148,8 +148,15 @@ http.createServer((req, res) => { host: TARGET_HOST, pathname: cacheKey, })); - // Cloudflare caches these 302s at the edge per UA cohort; Location depends only on the path. - res.setHeader('Cache-Control', MISSING_CACHE_CONTROL); + // Only Cloudflare may cache these redirects: its cache key (electron/infra + // cache ruleset) separates the redirect cohort on both triggers of this + // branch, so a cached 302 cannot leak to ordinary clients. Generic shared + // caches and browsers key on URL alone, so they get no-store, while + // Cloudflare-CDN-Cache-Control — Cloudflare-specific, preferred by + // Cloudflare over Cache-Control, and not forwarded downstream — keeps the + // edge caching the redirect for an hour. + res.setHeader('Cache-Control', 'no-store'); + res.setHeader('Cloudflare-CDN-Cache-Control', MISSING_CACHE_CONTROL); return res.writeHead(302).end(); } diff --git a/test/server.test.js b/test/server.test.js index cf1b047..6f025b7 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -221,7 +221,7 @@ test('upstream Cache-Control on 200s is preserved', async (t) => { assert.equal(res.headers['cache-control'], 'public, max-age=60'); }); -test('redirect responses are edge-cacheable', async (t) => { +test('redirect responses are cacheable by Cloudflare only', async (t) => { const server = await startSymbolServer({ targetHost: 'symbols.example.test' }); t.after(() => server.stop()); @@ -229,7 +229,10 @@ test('redirect responses are edge-cacheable', async (t) => { 'user-agent': 'symbolicator/1.2.3', }); assert.equal(res.statusCode, 302); - assert.equal(res.headers['cache-control'], 'public, max-age=3600'); + // Generic shared caches and browsers must never store the redirect... + assert.equal(res.headers['cache-control'], 'no-store'); + // ...while Cloudflare (whose cache key separates the redirect cohort) may. + assert.equal(res.headers['cloudflare-cdn-cache-control'], 'public, max-age=3600'); }); test('proxy returns 500 with error ID when upstream is unreachable', async (t) => {