Skip to content

Commit f028e72

Browse files
CopilotCopilot
authored
Refactor proxy request error path into shared handler (#3323)
* Initial plan * refactor: deduplicate proxy request error handling * test: stabilize flaky integration network checks * fix: address proxy request error-path review feedback --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
1 parent 6e59b62 commit f028e72

4 files changed

Lines changed: 224 additions & 39 deletions

File tree

‎containers/api-proxy/proxy-request.js‎

Lines changed: 63 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,35 @@ function isValidRequestId(id) {
119119
return typeof id === 'string' && id.length <= 128 && /^[\w\-\.]+$/.test(id);
120120
}
121121

122+
function handleRequestError(err, {
123+
res,
124+
requestId,
125+
provider,
126+
req,
127+
targetHost,
128+
startTime,
129+
statusCode,
130+
clientMessage,
131+
extraMetrics,
132+
onHeadersSent,
133+
}) {
134+
const duration = Date.now() - startTime;
135+
metrics.gaugeDec('active_requests', { provider });
136+
metrics.increment('requests_errors_total', { provider });
137+
if (extraMetrics) extraMetrics(duration);
138+
logRequest('error', 'request_error', {
139+
request_id: requestId, provider, method: req.method,
140+
path: sanitizeForLog(req.url), duration_ms: duration,
141+
error: sanitizeForLog(err.message), upstream_host: targetHost,
142+
});
143+
if (res.headersSent) {
144+
if (onHeadersSent) onHeadersSent(err);
145+
return;
146+
}
147+
res.writeHead(statusCode, { 'Content-Type': 'application/json' });
148+
res.end(JSON.stringify({ error: clientMessage, message: err.message }));
149+
}
150+
122151
const checkRateLimit = createRateLimitChecker({
123152
limiter,
124153
metrics,
@@ -201,16 +230,16 @@ function proxyRequest(req, res, targetHost, injectHeaders, provider, basePath =
201230
req.on('error', (err) => {
202231
if (errored) return;
203232
errored = true;
204-
const duration = Date.now() - startTime;
205-
metrics.gaugeDec('active_requests', { provider });
206-
metrics.increment('requests_errors_total', { provider });
207-
logRequest('error', 'request_error', {
208-
request_id: requestId, provider, method: req.method,
209-
path: sanitizeForLog(req.url), duration_ms: duration,
210-
error: sanitizeForLog(err.message), upstream_host: targetHost,
233+
handleRequestError(err, {
234+
res,
235+
requestId,
236+
provider,
237+
req,
238+
targetHost,
239+
startTime,
240+
statusCode: 400,
241+
clientMessage: 'Client error',
211242
});
212-
if (!res.headersSent) res.writeHead(400, { 'Content-Type': 'application/json' });
213-
res.end(JSON.stringify({ error: 'Client error', message: err.message }));
214243
});
215244

216245
req.on('data', chunk => {
@@ -356,16 +385,19 @@ function proxyRequest(req, res, targetHost, injectHeaders, provider, basePath =
356385
proxyRes.on('data', (chunk) => { responseBytes += chunk.length; });
357386

358387
proxyRes.on('error', (err) => {
359-
const duration = Date.now() - startTime;
360-
metrics.gaugeDec('active_requests', { provider });
361-
metrics.increment('requests_errors_total', { provider });
362-
logRequest('error', 'request_error', {
363-
request_id: requestId, provider, method: req.method,
364-
path: sanitizeForLog(req.url), duration_ms: duration,
365-
error: sanitizeForLog(err.message), upstream_host: targetHost,
388+
handleRequestError(err, {
389+
res,
390+
requestId,
391+
provider,
392+
req,
393+
targetHost,
394+
startTime,
395+
statusCode: 502,
396+
clientMessage: 'Response stream error',
397+
onHeadersSent: () => {
398+
if (typeof res.destroy === 'function') res.destroy(err);
399+
},
366400
});
367-
if (!res.headersSent) res.writeHead(502, { 'Content-Type': 'application/json' });
368-
res.end(JSON.stringify({ error: 'Response stream error', message: err.message }));
369401
});
370402

371403
const billingInfo = extractBillingHeaders(proxyRes.headers);
@@ -419,18 +451,20 @@ function proxyRequest(req, res, targetHost, injectHeaders, provider, basePath =
419451
});
420452

421453
proxyReq.on('error', (err) => {
422-
const duration = Date.now() - startTime;
423-
metrics.gaugeDec('active_requests', { provider });
424-
metrics.increment('requests_errors_total', { provider });
425-
metrics.increment('requests_total', { provider, method: req.method, status_class: '5xx' });
426-
metrics.observe('request_duration_ms', duration, { provider });
427-
logRequest('error', 'request_error', {
428-
request_id: requestId, provider, method: req.method,
429-
path: sanitizeForLog(req.url), duration_ms: duration,
430-
error: sanitizeForLog(err.message), upstream_host: targetHost,
454+
handleRequestError(err, {
455+
res,
456+
requestId,
457+
provider,
458+
req,
459+
targetHost,
460+
startTime,
461+
statusCode: 502,
462+
clientMessage: 'Proxy error',
463+
extraMetrics: (duration) => {
464+
metrics.increment('requests_total', { provider, method: req.method, status_class: '5xx' });
465+
metrics.observe('request_duration_ms', duration, { provider });
466+
},
431467
});
432-
if (!res.headersSent) res.writeHead(502, { 'Content-Type': 'application/json' });
433-
res.end(JSON.stringify({ error: 'Proxy error', message: err.message }));
434468
});
435469

436470
if (body.length > 0) proxyReq.write(body);

‎containers/api-proxy/server.proxy.test.js‎

Lines changed: 152 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,12 @@ const { resetEffectiveTokenGuardForTests, resetMaxRunsGuardForTests, resetTimeou
1414
const originalHttpsProxy = process.env.HTTPS_PROXY;
1515
let proxyRequest;
1616
let proxyWebSocket;
17+
let healthResponse;
1718

1819
beforeAll(() => {
1920
delete process.env.HTTPS_PROXY;
2021
jest.resetModules();
21-
({ proxyRequest, proxyWebSocket } = require('./server'));
22+
({ proxyRequest, proxyWebSocket, healthResponse } = require('./server'));
2223
});
2324

2425
afterAll(() => {
@@ -472,6 +473,156 @@ describe('proxyRequest X-Initiator injection', () => {
472473
});
473474
});
474475

476+
describe('proxyRequest error handling', () => {
477+
function makeReq(headers = {}) {
478+
const req = new EventEmitter();
479+
req.url = '/v1/chat/completions';
480+
req.method = 'POST';
481+
req.headers = { 'content-type': 'application/json', ...headers };
482+
return req;
483+
}
484+
485+
function makeRes() {
486+
const res = {
487+
headersSent: false,
488+
setHeader: jest.fn(),
489+
writeHead: jest.fn(() => {
490+
res.headersSent = true;
491+
}),
492+
end: jest.fn(),
493+
destroy: jest.fn(),
494+
};
495+
return res;
496+
}
497+
498+
function getRequestErrorLog(writeSpy) {
499+
for (const [line] of writeSpy.mock.calls) {
500+
try {
501+
const parsed = JSON.parse(line);
502+
if (parsed.event === 'request_error') return parsed;
503+
} catch {
504+
// ignore non-JSON writes
505+
}
506+
}
507+
return null;
508+
}
509+
510+
let stdoutWriteSpy;
511+
512+
beforeEach(() => {
513+
stdoutWriteSpy = jest.spyOn(process.stdout, 'write').mockImplementation(() => true);
514+
});
515+
516+
afterEach(() => {
517+
jest.restoreAllMocks();
518+
});
519+
520+
it('returns 400 when the client request stream errors', () => {
521+
const before = healthResponse().metrics_summary;
522+
const req = makeReq();
523+
const res = makeRes();
524+
525+
proxyRequest(req, res, 'api.openai.com', { Authorization: 'Bearer token' }, 'openai');
526+
req.emit('error', new Error('client stream failed\ninjected'));
527+
528+
expect(res.writeHead).toHaveBeenCalledWith(400, { 'Content-Type': 'application/json' });
529+
expect(JSON.parse(res.end.mock.calls[0][0])).toEqual({
530+
error: 'Client error',
531+
message: 'client stream failed\ninjected',
532+
});
533+
const after = healthResponse().metrics_summary;
534+
expect(after.total_errors).toBe(before.total_errors + 1);
535+
expect(after.active_requests).toBe(before.active_requests);
536+
const errorLog = getRequestErrorLog(stdoutWriteSpy);
537+
expect(errorLog).toMatchObject({
538+
event: 'request_error',
539+
provider: 'openai',
540+
method: 'POST',
541+
path: '/v1/chat/completions',
542+
error: 'client stream failedinjected',
543+
upstream_host: 'api.openai.com',
544+
});
545+
});
546+
547+
it('destroys response when upstream response stream errors after headers are sent', () => {
548+
const before = healthResponse().metrics_summary;
549+
let responseHandler;
550+
const upstreamRequest = new EventEmitter();
551+
upstreamRequest.end = jest.fn();
552+
upstreamRequest.write = jest.fn();
553+
upstreamRequest.destroy = jest.fn();
554+
555+
jest.spyOn(https, 'request').mockImplementation((_options, cb) => {
556+
responseHandler = cb;
557+
return upstreamRequest;
558+
});
559+
560+
const req = makeReq();
561+
const res = makeRes();
562+
proxyRequest(req, res, 'api.openai.com', { Authorization: 'Bearer token' }, 'openai');
563+
req.emit('end');
564+
565+
const proxyRes = new EventEmitter();
566+
proxyRes.statusCode = 200;
567+
proxyRes.headers = {};
568+
proxyRes.pipe = jest.fn();
569+
responseHandler(proxyRes);
570+
proxyRes.emit('error', new Error('upstream stream failed'));
571+
572+
expect(res.writeHead).toHaveBeenNthCalledWith(1, 200, { 'x-request-id': expect.any(String) });
573+
expect(res.writeHead).toHaveBeenCalledTimes(1);
574+
expect(res.end).not.toHaveBeenCalled();
575+
expect(res.destroy).toHaveBeenCalledWith(expect.any(Error));
576+
const after = healthResponse().metrics_summary;
577+
expect(after.total_errors).toBe(before.total_errors + 1);
578+
expect(after.active_requests).toBe(before.active_requests);
579+
const errorLog = getRequestErrorLog(stdoutWriteSpy);
580+
expect(errorLog).toMatchObject({
581+
event: 'request_error',
582+
provider: 'openai',
583+
method: 'POST',
584+
path: '/v1/chat/completions',
585+
error: 'upstream stream failed',
586+
upstream_host: 'api.openai.com',
587+
});
588+
});
589+
590+
it('returns 502 when the upstream proxy request errors', () => {
591+
const before = healthResponse().metrics_summary;
592+
const upstreamRequest = new EventEmitter();
593+
upstreamRequest.end = jest.fn();
594+
upstreamRequest.write = jest.fn();
595+
upstreamRequest.destroy = jest.fn();
596+
597+
jest.spyOn(https, 'request').mockImplementation(() => upstreamRequest);
598+
599+
const req = makeReq();
600+
const res = makeRes();
601+
proxyRequest(req, res, 'api.openai.com', { Authorization: 'Bearer token' }, 'openai');
602+
req.emit('end');
603+
upstreamRequest.emit('error', new Error('upstream connect failed'));
604+
605+
expect(res.writeHead).toHaveBeenCalledWith(502, { 'Content-Type': 'application/json' });
606+
expect(JSON.parse(res.end.mock.calls[0][0])).toEqual({
607+
error: 'Proxy error',
608+
message: 'upstream connect failed',
609+
});
610+
const after = healthResponse().metrics_summary;
611+
expect(after.total_errors).toBe(before.total_errors + 1);
612+
expect(after.total_requests).toBe(before.total_requests + 1);
613+
expect(after.active_requests).toBe(before.active_requests);
614+
const errorLog = getRequestErrorLog(stdoutWriteSpy);
615+
expect(errorLog).toMatchObject({
616+
event: 'request_error',
617+
provider: 'openai',
618+
method: 'POST',
619+
path: '/v1/chat/completions',
620+
error: 'upstream connect failed',
621+
upstream_host: 'api.openai.com',
622+
});
623+
});
624+
});
625+
475626
describe('proxyRequest effective token guard', () => {
476627
function makeReq(headers = {}) {
477628
const req = new EventEmitter();

‎tests/integration/one-shot-tokens.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ describe('One-Shot Token Protection', () => {
6565
{
6666
allowDomains: ['localhost'],
6767
logLevel: 'debug',
68-
timeout: 240000,
68+
timeout: 480000,
6969
buildLocal: true, // Build container locally to include one-shot-token.so
7070
env: {
7171
GITHUB_TOKEN: 'ghp_test_token_12345',
@@ -80,7 +80,7 @@ describe('One-Shot Token Protection', () => {
8080
expect(result.stdout).toContain('Second read: [ghp_test_token_12345]');
8181
// Note: printenv reads from environ array directly, not via getenv().
8282
// The LD_PRELOAD library only intercepts getenv() calls, so no debug output appears here.
83-
}, 240000);
83+
}, 480000);
8484

8585
test('should cache COPILOT_GITHUB_TOKEN and clear from environment', async () => {
8686
const testScript = `

‎tests/integration/protocol-support.test.ts‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ describe('Protocol Support', () => {
2929
describe('HTTPS Connections', () => {
3030
test('should allow HTTPS to allowed domain', async () => {
3131
const result = await runner.runWithSudo(
32-
'curl -fsS https://api.github.com/zen',
32+
'curl -fsS https://github.com',
3333
{
3434
allowDomains: ['github.com'],
3535
logLevel: 'debug',
@@ -55,7 +55,7 @@ describe('Protocol Support', () => {
5555

5656
test('should handle HTTPS with verbose output', async () => {
5757
const result = await runner.runWithSudo(
58-
'curl -v https://api.github.com/zen 2>&1 | grep -E "SSL|TLS" | head -5 || true',
58+
'curl -v https://github.com 2>&1 | grep -E "SSL|TLS" | head -5 || true',
5959
{
6060
allowDomains: ['github.com'],
6161
logLevel: 'debug',
@@ -117,7 +117,7 @@ describe('Protocol Support', () => {
117117
describe('Connection Headers', () => {
118118
test('should pass custom headers', async () => {
119119
const result = await runner.runWithSudo(
120-
'curl -fsS -H "Accept: application/vnd.github+json" https://api.github.com/zen',
120+
'curl -fsS -H "Accept: text/html" https://github.com',
121121
{
122122
allowDomains: ['github.com'],
123123
logLevel: 'debug',
@@ -130,7 +130,7 @@ describe('Protocol Support', () => {
130130

131131
test('should pass User-Agent header', async () => {
132132
const result = await runner.runWithSudo(
133-
'curl -fsS -A "Test-Agent/1.0" https://api.github.com/zen',
133+
'curl -fsS -A "Test-Agent/1.0" https://github.com',
134134
{
135135
allowDomains: ['github.com'],
136136
logLevel: 'debug',
@@ -145,7 +145,7 @@ describe('Protocol Support', () => {
145145
describe('IPv4/IPv6', () => {
146146
test('should support IPv4 connections', async () => {
147147
const result = await runner.runWithSudo(
148-
'curl -fsS -4 https://api.github.com/zen',
148+
'curl -fsS -4 https://github.com',
149149
{
150150
allowDomains: ['github.com'],
151151
logLevel: 'debug',
@@ -159,7 +159,7 @@ describe('Protocol Support', () => {
159159
test('should handle IPv6 (may not be available)', async () => {
160160
// IPv6 may not be available in all environments
161161
const result = await runner.runWithSudo(
162-
'curl -fsS -6 https://api.github.com/zen || exit 0',
162+
'curl -fsS -6 https://github.com || exit 0',
163163
{
164164
allowDomains: ['github.com'],
165165
logLevel: 'debug',
@@ -175,7 +175,7 @@ describe('Protocol Support', () => {
175175
describe('Connection Timeouts', () => {
176176
test('should respect curl max-time option', async () => {
177177
const result = await runner.runWithSudo(
178-
'curl -f --max-time 5 https://api.github.com/zen',
178+
'curl -f --max-time 5 https://github.com',
179179
{
180180
allowDomains: ['github.com'],
181181
logLevel: 'debug',

0 commit comments

Comments
 (0)