Skip to content

Commit fa799ea

Browse files
Mercy811claude
andauthored
fix(analytics-node): add a request timeout to the HTTP transport (#1923)
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent a53cfc3 commit fa799ea

6 files changed

Lines changed: 430 additions & 138 deletions

File tree

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
11
import { IConfig } from './core-config';
22

3-
export type NodeConfig = IConfig;
3+
export interface NodeConfig extends IConfig {
4+
/**
5+
* The maximum time in milliseconds an event upload may stay idle before it is
6+
* aborted and retried. Guards against uploads that never complete.
7+
*/
8+
requestTimeoutMillis: number;
9+
}
410

511
export type NodeOptions = Omit<Partial<NodeConfig>, 'apiKey'>;

packages/analytics-node/src/config.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,17 @@
11
import { Config, NodeOptions, NodeConfig as INodeConfig } from '@amplitude/analytics-core';
2-
import { Http } from './transports/http';
2+
import { DEFAULT_REQUEST_TIMEOUT_MILLIS, Http } from './transports/http';
33

44
export class NodeConfig extends Config implements INodeConfig {
5+
requestTimeoutMillis: number;
6+
57
constructor(apiKey: string, options?: NodeOptions) {
8+
const requestTimeoutMillis = options?.requestTimeoutMillis ?? DEFAULT_REQUEST_TIMEOUT_MILLIS;
69
super({
7-
transportProvider: new Http(),
10+
transportProvider: new Http(requestTimeoutMillis),
811
...options,
912
apiKey,
1013
});
14+
this.requestTimeoutMillis = requestTimeoutMillis;
1115
}
1216
}
1317

packages/analytics-node/src/transports/http.ts

Lines changed: 70 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,24 @@ import { BaseTransport, Payload, Response, Transport } from '@amplitude/analytic
22
import * as http from 'http';
33
import * as https from 'https';
44

5+
/**
6+
* Node's HTTP client applies no timeout of its own, so without this a stalled or
7+
* slow-trickling upload hangs forever. Matches the 10s network timeout used by
8+
* the Amplitude Java and Python SDKs.
9+
*/
10+
export const DEFAULT_REQUEST_TIMEOUT_MILLIS = 10000;
11+
12+
// buildResponse() maps 408 to Status.Timeout and 0 to Status.Unknown. Destination
13+
// retries both with backoff, bounded by flushMaxRetries, rather than dropping the
14+
// batch the way it does for a null response.
15+
const REQUEST_TIMEOUT_STATUS_CODE = 408;
16+
const INCOMPLETE_RESPONSE_STATUS_CODE = 0;
17+
518
export class Http extends BaseTransport implements Transport {
19+
constructor(private readonly requestTimeoutMillis: number = DEFAULT_REQUEST_TIMEOUT_MILLIS) {
20+
super();
21+
}
22+
623
send(serverUrl: string, payload: Payload): Promise<Response | null> {
724
let protocol: typeof http | typeof https;
825
if (serverUrl.startsWith('http://')) {
@@ -27,27 +44,70 @@ export class Http extends BaseTransport implements Transport {
2744
protocol: url.protocol,
2845
};
2946
return new Promise((resolve) => {
47+
// Every path below must settle exactly once. A send() that never settles
48+
// leaves Destination.flushId set forever, which silently no-ops all later
49+
// flushes while the event queue keeps growing. See SDK-188.
50+
let settled = false;
51+
// `timer` is declared below, once req exists. Safe to close over here
52+
// because protocol.request() never invokes its response callback
53+
// synchronously, so nothing can call settle() before that runs.
54+
const settle = (response: Response | null) => {
55+
if (settled) {
56+
return;
57+
}
58+
settled = true;
59+
clearTimeout(timer.deadline);
60+
resolve(response);
61+
};
62+
63+
// A connection that drops mid-response is transient, so retry rather than
64+
// discard the batch. Amplitude dedupes on insert_id, making a replay of a
65+
// batch the server may already have processed safe.
66+
const settleAsIncomplete = () => settle(this.buildResponse({ code: INCOMPLETE_RESPONSE_STATUS_CODE }));
67+
3068
const req = protocol.request(options, (res) => {
3169
res.setEncoding('utf8');
3270
let responsePayload = '';
3371
res.on('data', (chunk: string) => {
3472
responsePayload += chunk;
3573
});
74+
res.on('aborted', settleAsIncomplete);
75+
res.on('error', settleAsIncomplete);
3676

3777
res.on('end', () => {
38-
if (res.complete && responsePayload.length > 0) {
39-
try {
40-
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
41-
const parsedResponsePayload: Record<string, any> = JSON.parse(responsePayload);
42-
const result = this.buildResponse(parsedResponsePayload);
43-
resolve(result);
44-
} catch {
45-
resolve(this.buildResponse({ code: res.statusCode }));
46-
}
78+
// A truncated body tells us nothing about whether the server accepted
79+
// the batch.
80+
if (!res.complete) {
81+
settleAsIncomplete();
82+
return;
83+
}
84+
try {
85+
// An empty body lands here too, and falls back to the status line.
86+
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
87+
const parsedResponsePayload: Record<string, any> = JSON.parse(responsePayload);
88+
settle(this.buildResponse(parsedResponsePayload));
89+
} catch {
90+
settle(this.buildResponse({ code: res.statusCode }));
4791
}
4892
});
4993
});
50-
req.on('error', () => resolve(null));
94+
95+
// Unlike the cases above, a request that never reached the server keeps its
96+
// long-standing drop-on-error behavior.
97+
req.on('error', () => settle(null));
98+
99+
// An absolute deadline, not req.setTimeout()'s socket-inactivity timer:
100+
// that resets on every byte of traffic, so a response trickled slower than
101+
// requestTimeoutMillis but never finished would keep it from firing at all.
102+
// unref() so it can't keep the event loop alive on its own.
103+
const timer = {
104+
deadline: setTimeout(() => {
105+
req.destroy();
106+
settle(this.buildResponse({ code: REQUEST_TIMEOUT_STATUS_CODE }));
107+
}, this.requestTimeoutMillis),
108+
};
109+
timer.deadline.unref();
110+
51111
req.end(requestPayload);
52112
});
53113
}

packages/analytics-node/test/config.test.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ describe('config', () => {
2020
loggerProvider: logger,
2121
logLevel: LogLevel.Warn,
2222
minIdLength: undefined,
23+
requestTimeoutMillis: 10000,
2324
offline: false,
2425
_optOut: false,
2526
plan: undefined,
@@ -31,6 +32,12 @@ describe('config', () => {
3132
useBatch: false,
3233
});
3334
});
35+
36+
test('should overwrite the request timeout', () => {
37+
const config = new Config.NodeConfig(API_KEY, { requestTimeoutMillis: 500 });
38+
expect(config.requestTimeoutMillis).toBe(500);
39+
expect(config.transportProvider).toEqual(new Http(500));
40+
});
3441
});
3542

3643
describe('useNodeConfig', () => {
@@ -47,6 +54,7 @@ describe('config', () => {
4754
loggerProvider: logger,
4855
logLevel: LogLevel.Warn,
4956
minIdLength: undefined,
57+
requestTimeoutMillis: 10000,
5058
offline: false,
5159
_optOut: false,
5260
plan: undefined,
Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
import { Destination, Event, LogLevel } from '@amplitude/analytics-core';
2+
import * as http from 'http';
3+
import { AddressInfo } from 'net';
4+
import { NodeConfig } from '../src/config';
5+
6+
/**
7+
* SDK-188: an upload that never settles used to pin Destination.flushId, which
8+
* made every later flush a no-op while execute() kept appending to an uncapped
9+
* queue. These assert the queue drains no matter how the upload fails.
10+
*/
11+
describe('destination integration: upload timeout', () => {
12+
let server: http.Server;
13+
let serverUrl: string;
14+
let requestCount: number;
15+
16+
let uploadsPerEvent: Map<string, number>;
17+
18+
const listen = async (handler: http.RequestListener) => {
19+
requestCount = 0;
20+
uploadsPerEvent = new Map();
21+
server = http.createServer((req, res) => {
22+
const chunks: Buffer[] = [];
23+
req.on('data', (chunk: Buffer) => chunks.push(chunk));
24+
req.on('end', () => {
25+
requestCount += 1;
26+
const body = JSON.parse(Buffer.concat(chunks).toString()) as { events: { insert_id: string }[] };
27+
body.events.forEach(({ insert_id }) =>
28+
uploadsPerEvent.set(insert_id, (uploadsPerEvent.get(insert_id) ?? 0) + 1),
29+
);
30+
handler(req, res);
31+
});
32+
});
33+
await new Promise<void>((resolve) => server.listen(0, resolve));
34+
serverUrl = `http://localhost:${(server.address() as AddressInfo).port}/2/httpapi`;
35+
};
36+
37+
const setupDestination = async (overrides = {}) => {
38+
const config = new NodeConfig('a'.repeat(32), {
39+
serverUrl,
40+
requestTimeoutMillis: 50,
41+
flushIntervalMillis: 10,
42+
flushQueueSize: 5,
43+
logLevel: LogLevel.None,
44+
...overrides,
45+
});
46+
const destination = new Destination();
47+
destination.retryTimeout = 10;
48+
await destination.setup(config);
49+
return destination;
50+
};
51+
52+
const events = (count: number): Event[] =>
53+
Array.from({ length: count }, (_, i) => ({ event_type: 'exposure', insert_id: `id-${i}` }));
54+
55+
// Callbacks fire inside send(), one tick before flush() clears flushId. Poll
56+
// instead of asserting immediately; a flushId that never clears is the bug.
57+
const waitForFlushToRelease = async (destination: Destination) => {
58+
for (let i = 0; i < 200 && destination.flushId !== null; i++) {
59+
await new Promise((resolve) => setTimeout(resolve, 10));
60+
}
61+
};
62+
63+
afterEach(async () => {
64+
server.closeAllConnections();
65+
await new Promise((resolve) => server.close(resolve));
66+
});
67+
68+
test('should recover once a stalled upload times out', async () => {
69+
await listen((_, res) => {
70+
// Hold the first upload open forever, then behave normally.
71+
if (requestCount === 1) {
72+
return;
73+
}
74+
res.writeHead(200, { 'Content-Type': 'application/json' });
75+
res.end(JSON.stringify({ code: 200 }));
76+
});
77+
const destination = await setupDestination();
78+
79+
const results = await Promise.all(events(20).map((event) => destination.execute(event)));
80+
81+
expect(requestCount).toBeGreaterThan(1);
82+
expect(results.every((result) => result.code === 200)).toBe(true);
83+
await waitForFlushToRelease(destination);
84+
85+
expect(destination.queue).toHaveLength(0);
86+
expect(destination.flushId).toBeNull();
87+
});
88+
89+
test('should drain the queue when every upload stalls', async () => {
90+
await listen(() => {
91+
// Never respond to anything.
92+
});
93+
const destination = await setupDestination({ flushMaxRetries: 2 });
94+
95+
const results = await Promise.all(events(20).map((event) => destination.execute(event)));
96+
97+
expect(results.every((result) => result.code === 500)).toBe(true);
98+
await waitForFlushToRelease(destination);
99+
100+
expect(destination.queue).toHaveLength(0);
101+
expect(destination.flushId).toBeNull();
102+
});
103+
104+
test('should retry rather than drop when uploads time out', async () => {
105+
await listen(() => {
106+
// Never respond to anything.
107+
});
108+
const destination = await setupDestination({ flushMaxRetries: 4 });
109+
110+
const results = await Promise.all(events(10).map((event) => destination.execute(event)));
111+
112+
// Held in memory and re-uploaded until flushMaxRetries is exhausted, rather
113+
// than discarded after the first failed attempt.
114+
expect([...uploadsPerEvent.values()]).toEqual(Array(10).fill(4));
115+
expect(results.every((result) => result.message === 'Event rejected due to exceeded retry count')).toBe(true);
116+
});
117+
118+
test('should retry rather than drop when the response is truncated', async () => {
119+
await listen((_, res) => {
120+
res.writeHead(200, { 'Content-Type': 'application/json', 'Content-Length': '200' });
121+
res.write('{"code":200,"events_ing');
122+
setTimeout(() => res.socket?.destroy(), 5);
123+
});
124+
const destination = await setupDestination({ flushMaxRetries: 3 });
125+
126+
const results = await Promise.all(events(10).map((event) => destination.execute(event)));
127+
128+
expect([...uploadsPerEvent.values()]).toEqual(Array(10).fill(3));
129+
expect(results.every((result) => result.message === 'Event rejected due to exceeded retry count')).toBe(true);
130+
});
131+
132+
test('should drain the queue when uploads return an empty body', async () => {
133+
await listen((_, res) => {
134+
res.writeHead(200, { 'Content-Length': '0' });
135+
res.end();
136+
});
137+
const destination = await setupDestination();
138+
139+
const results = await Promise.all(events(20).map((event) => destination.execute(event)));
140+
141+
expect(results.every((result) => result.code === 200)).toBe(true);
142+
await waitForFlushToRelease(destination);
143+
144+
expect(destination.queue).toHaveLength(0);
145+
expect(destination.flushId).toBeNull();
146+
});
147+
});

0 commit comments

Comments
 (0)