Skip to content

Commit f960de8

Browse files
Reflexclaude
Reflex
and
claude
committed
test(h2-transport): address review feedback
Inline review comments on #806 from codeant-ai. Each addressed: helpers/certs.ts: - Switch from execSync with shelled-out openssl to execFileSync (argv array) so paths containing spaces or shell metacharacters don't break cert gen. helpers/testServer.ts: - goawayAll now has a 1s fallback that destroys sessions if the peer doesn't close promptly, so the promise can't hang indefinitely. - startBlackholeServer.close returns Promise<void> so test teardown can await actual shutdown completion. session.test.ts: - maxConcurrentStreams adoption tests poll for the expected state instead of using a fixed 50ms sleep — non-flaky on slower CI. - abort-listener tests use an instrumented AbortSignal that counts addEventListener/removeEventListener calls, proving the listener is removed on both success and mid-flight-abort paths. Added a second test for the abort path. regression.test.ts: - R12 polls for _maxConcurrentStreams instead of sleeping. integration.test.ts: - 'concurrent origins' and 'AbortSignal' tests put fetch.close() in finally so the H2 pool can't leak on assertion failure. loadtest/h2load.sh: - Run the server in its own process group via setsid; cleanup kills the whole group so npx + node both die. - Pass -k to h2load so the self-signed cert handshake doesn't fail. loadtest/h2-multiplex.ts: - Report both attemptedRps (N / wall) and effectiveRps (successful / wall) so failures can't inflate the throughput number. - Cert gen via execFileSync. loadtest/h2-pool-growth.ts: - Sample actual pool _sessions.length (drive via H2Pool directly since createH2Fetch's per-origin map is closure-private), and assert grow-on-ramp / no-shrink / bounded-by-maxConnections invariants. loadtest/h2-chaos.ts: - Validate durationSec > 0; guard against zero-denominator NaN in the success-rate check. - Cert gen via execFileSync. loadtest/h2-leak.ts, h2-multiplex-server.ts: - Cert gen via execFileSync. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 9f34ab3 commit f960de8

11 files changed

Lines changed: 236 additions & 84 deletions

File tree

‎loadtest/h2-chaos.ts‎

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
* Run: `npx tsx loadtest/h2-chaos.ts [durationSeconds=60]`
66
*/
77
import http2 from 'node:http2';
8-
import { execSync } from 'node:child_process';
8+
import { execFileSync } from 'node:child_process';
99
import fs from 'node:fs';
1010
import os from 'node:os';
1111
import path from 'node:path';
@@ -17,8 +17,11 @@ function makeCerts() {
1717
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'h2-chaos-'));
1818
const key = path.join(tmp, 'key.pem');
1919
const cert = path.join(tmp, 'cert.pem');
20-
execSync(
21-
`openssl req -x509 -newkey rsa:2048 -keyout ${key} -out ${cert} -days 1 -nodes -subj "/CN=localhost" 2>/dev/null`,
20+
execFileSync(
21+
'openssl',
22+
['req', '-x509', '-newkey', 'rsa:2048', '-keyout', key, '-out', cert,
23+
'-days', '1', '-nodes', '-subj', '/CN=localhost'],
24+
{ stdio: ['ignore', 'ignore', 'ignore'] },
2225
);
2326
return { key: fs.readFileSync(key), cert: fs.readFileSync(cert), tmp };
2427
}
@@ -76,6 +79,10 @@ async function startServer(seed: number) {
7679

7780
async function main() {
7881
const durationSec = Number(process.argv[2] ?? 60);
82+
if (!Number.isFinite(durationSec) || durationSec <= 0) {
83+
console.error(`invalid duration: ${process.argv[2]} (must be a positive number of seconds)`);
84+
process.exit(2);
85+
}
7986
const server = await startServer(42);
8087
const fetch = createH2Fetch({
8188
minConnections: 4,
@@ -119,17 +126,25 @@ async function main() {
119126
await fetch.close();
120127
server.close();
121128

122-
const getRate = getOk / (getOk + getFail);
129+
const getTotal = getOk + getFail;
130+
const postTotal = postOk + postFail;
131+
if (getTotal === 0) {
132+
console.error('no GET requests were issued — load harness produced zero coverage');
133+
process.exit(1);
134+
}
135+
const getRate = getOk / getTotal;
123136
const postClean = postFail === 0 || postOk > 0;
124137

125138
console.log(
126139
JSON.stringify(
127140
{
128141
getOk,
129142
getFail,
143+
getTotal,
130144
getSuccessRate: Number(getRate.toFixed(3)),
131145
postOk,
132146
postFail,
147+
postTotal,
133148
durationSec,
134149
},
135150
null,

‎loadtest/h2-leak.ts‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
* Run: `npx tsx loadtest/h2-leak.ts [durationSeconds=600]`
66
*/
77
import http2 from 'node:http2';
8-
import { execSync } from 'node:child_process';
8+
import { execFileSync } from 'node:child_process';
99
import fs from 'node:fs';
1010
import os from 'node:os';
1111
import path from 'node:path';
@@ -17,8 +17,11 @@ function makeCerts() {
1717
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'h2-leak-'));
1818
const key = path.join(tmp, 'key.pem');
1919
const cert = path.join(tmp, 'cert.pem');
20-
execSync(
21-
`openssl req -x509 -newkey rsa:2048 -keyout ${key} -out ${cert} -days 1 -nodes -subj "/CN=localhost" 2>/dev/null`,
20+
execFileSync(
21+
'openssl',
22+
['req', '-x509', '-newkey', 'rsa:2048', '-keyout', key, '-out', cert,
23+
'-days', '1', '-nodes', '-subj', '/CN=localhost'],
24+
{ stdio: ['ignore', 'ignore', 'ignore'] },
2225
);
2326
return { key: fs.readFileSync(key), cert: fs.readFileSync(cert), tmp };
2427
}

‎loadtest/h2-multiplex-server.ts‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,16 +3,19 @@
33
* Logs its port on startup and serves a 1KB JSON body for every request.
44
*/
55
import http2 from 'node:http2';
6-
import { execSync } from 'node:child_process';
6+
import { execFileSync } from 'node:child_process';
77
import fs from 'node:fs';
88
import os from 'node:os';
99
import path from 'node:path';
1010

1111
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'h2load-srv-'));
1212
const keyPath = path.join(tmp, 'key.pem');
1313
const certPath = path.join(tmp, 'cert.pem');
14-
execSync(
15-
`openssl req -x509 -newkey rsa:2048 -keyout ${keyPath} -out ${certPath} -days 1 -nodes -subj "/CN=localhost" 2>/dev/null`,
14+
execFileSync(
15+
'openssl',
16+
['req', '-x509', '-newkey', 'rsa:2048', '-keyout', keyPath, '-out', certPath,
17+
'-days', '1', '-nodes', '-subj', '/CN=localhost'],
18+
{ stdio: ['ignore', 'ignore', 'ignore'] },
1619
);
1720
const payload = Buffer.from(JSON.stringify({ ok: true, data: 'x'.repeat(1000) }));
1821

‎loadtest/h2-multiplex.ts‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
* Run: `npx tsx loadtest/h2-multiplex.ts [N=1000]`
99
*/
1010
import http2 from 'node:http2';
11-
import { execSync } from 'node:child_process';
11+
import { execFileSync } from 'node:child_process';
1212
import fs from 'node:fs';
1313
import os from 'node:os';
1414
import path from 'node:path';
@@ -20,8 +20,11 @@ function makeCerts() {
2020
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'h2-mux-'));
2121
const key = path.join(tmp, 'key.pem');
2222
const cert = path.join(tmp, 'cert.pem');
23-
execSync(
24-
`openssl req -x509 -newkey rsa:2048 -keyout ${key} -out ${cert} -days 1 -nodes -subj "/CN=localhost" 2>/dev/null`,
23+
execFileSync(
24+
'openssl',
25+
['req', '-x509', '-newkey', 'rsa:2048', '-keyout', key, '-out', cert,
26+
'-days', '1', '-nodes', '-subj', '/CN=localhost'],
27+
{ stdio: ['ignore', 'ignore', 'ignore'] },
2528
);
2629
return { key: fs.readFileSync(key), cert: fs.readFileSync(cert), tmp };
2730
}
@@ -85,13 +88,16 @@ async function main() {
8588
const totalMs = Number(process.hrtime.bigint() - t0) / 1e6;
8689

8790
latencies.sort((a, b) => a - b);
91+
const successful = latencies.length;
8892
console.log(
8993
JSON.stringify(
9094
{
9195
N,
96+
successful,
9297
failures,
9398
totalMs: Math.round(totalMs),
94-
rps: Math.round((N / totalMs) * 1000),
99+
attemptedRps: Math.round((N / totalMs) * 1000),
100+
effectiveRps: Math.round((successful / totalMs) * 1000),
95101
p50_us: Math.round(percentile(latencies, 50)),
96102
p95_us: Math.round(percentile(latencies, 95)),
97103
p99_us: Math.round(percentile(latencies, 99)),

‎loadtest/h2-pool-growth.ts‎

Lines changed: 69 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,34 @@
11
/**
2-
* Pool growth dynamics — ramp concurrency up then down, log pool size over time.
2+
* Pool growth dynamics — ramp concurrency up then down against a real H2Pool,
3+
* sample actual session count over time, then assert grow-on-ramp /
4+
* no-shrink / bounded-by-maxConnections behavior.
35
*
4-
* Server advertises maxConcurrentStreams=10. Asserts the pool grows on
5-
* ramp-up and *does not* shrink back down (current implementation has no
6-
* shrinking — documented behavior).
6+
* Server advertises maxConcurrentStreams=10. We drive the pool directly
7+
* (instead of through createH2Fetch) so we can read `_sessions.length` —
8+
* createH2Fetch's per-origin pool map is closure-private.
79
*
810
* Run: `npx tsx loadtest/h2-pool-growth.ts`
911
*/
1012
import http2 from 'node:http2';
11-
import { execSync } from 'node:child_process';
13+
import { execFileSync } from 'node:child_process';
1214
import fs from 'node:fs';
1315
import os from 'node:os';
1416
import path from 'node:path';
15-
import { createH2Fetch } from '../src/lib/h2-transport/index';
17+
import { H2Pool } from '../src/lib/h2-transport/pool';
1618

1719
process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0';
1820

21+
const MAX_CONNECTIONS = 50;
22+
1923
function makeCerts() {
2024
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'h2-grow-'));
2125
const key = path.join(tmp, 'key.pem');
2226
const cert = path.join(tmp, 'cert.pem');
23-
execSync(
24-
`openssl req -x509 -newkey rsa:2048 -keyout ${key} -out ${cert} -days 1 -nodes -subj "/CN=localhost" 2>/dev/null`,
27+
execFileSync(
28+
'openssl',
29+
['req', '-x509', '-newkey', 'rsa:2048', '-keyout', key, '-out', cert,
30+
'-days', '1', '-nodes', '-subj', '/CN=localhost'],
31+
{ stdio: ['ignore', 'ignore', 'ignore'] },
2532
);
2633
return { key: fs.readFileSync(key), cert: fs.readFileSync(cert), tmp };
2734
}
@@ -52,30 +59,42 @@ async function startServer() {
5259

5360
async function main() {
5461
const server = await startServer();
55-
const fetch = createH2Fetch({
62+
const pool = new H2Pool(`https://localhost:${server.port}`, {
5663
minConnections: 1,
57-
maxConnections: 50,
64+
maxConnections: MAX_CONNECTIONS,
5865
tlsOptions: { rejectUnauthorized: false },
5966
});
6067

61-
const observations: Array<{ t: number; concurrency: number }> = [];
68+
const observations: Array<{ t: number; concurrency: number; sessions: number }> = [];
6269
const t0 = Date.now();
6370
let inFlight = 0;
71+
const stages: Record<string, { peakSessions: number; peakConcurrency: number }> = {};
72+
let currentStage = '';
73+
74+
const sessionsNow = () => (pool as any)._sessions.length as number;
6475

6576
const sample = () => {
66-
observations.push({ t: Date.now() - t0, concurrency: inFlight });
77+
const sessions = sessionsNow();
78+
observations.push({ t: Date.now() - t0, concurrency: inFlight, sessions });
79+
if (currentStage) {
80+
const s = stages[currentStage]!;
81+
s.peakSessions = Math.max(s.peakSessions, sessions);
82+
s.peakConcurrency = Math.max(s.peakConcurrency, inFlight);
83+
}
6784
};
6885
const sampler = setInterval(sample, 100);
6986

70-
async function burst(target: number, durationMs: number): Promise<void> {
87+
async function burst(name: string, target: number, durationMs: number): Promise<void> {
88+
currentStage = name;
89+
stages[name] = { peakSessions: 0, peakConcurrency: 0 };
7190
const end = Date.now() + durationMs;
7291
const ongoing = new Set<Promise<unknown>>();
7392
while (Date.now() < end) {
7493
while (ongoing.size < target && Date.now() < end) {
7594
inFlight++;
7695
const p = (async () => {
7796
try {
78-
const r = (await fetch(`https://localhost:${server.port}/x`, { method: 'GET' } as any)) as any;
97+
const r = await pool.request('/x', 'GET', {}, null);
7998
await r.text();
8099
} finally {
81100
inFlight--;
@@ -87,29 +106,49 @@ async function main() {
87106
await new Promise((r) => setTimeout(r, 10));
88107
}
89108
await Promise.all(ongoing);
109+
currentStage = '';
90110
}
91111

92112
console.log('ramp: 1 → 50 → 200 → 50 → 1');
93-
await burst(1, 2_000);
94-
await burst(50, 5_000);
95-
await burst(200, 8_000);
96-
await burst(50, 5_000);
97-
await burst(1, 5_000);
113+
await burst('s1_low', 1, 2_000);
114+
await burst('s2_mid', 50, 5_000);
115+
await burst('s3_peak', 200, 8_000);
116+
await burst('s4_back_mid', 50, 5_000);
117+
await burst('s5_back_low', 1, 5_000);
98118

99119
clearInterval(sampler);
100-
await fetch.close();
120+
const finalSessions = sessionsNow();
121+
await pool.close();
101122
server.close();
102123

103-
console.log(
104-
JSON.stringify(
105-
{
106-
note: 'pool never shrinks today; peak == final is expected',
107-
samples: observations.length,
108-
},
109-
null,
110-
2,
111-
),
112-
);
124+
const result = {
125+
stages,
126+
peakSessionsOverall: Math.max(...observations.map((o) => o.sessions)),
127+
finalSessions,
128+
samples: observations.length,
129+
};
130+
console.log(JSON.stringify(result, null, 2));
131+
132+
const failures: string[] = [];
133+
if (stages.s3_peak!.peakSessions <= stages.s1_low!.peakSessions) {
134+
failures.push(
135+
`pool did not grow on ramp-up: low=${stages.s1_low!.peakSessions} peak=${stages.s3_peak!.peakSessions}`,
136+
);
137+
}
138+
if (finalSessions < stages.s3_peak!.peakSessions) {
139+
failures.push(
140+
`pool shrank after ramp-down: peak=${stages.s3_peak!.peakSessions} final=${finalSessions} (no-shrink is documented behavior)`,
141+
);
142+
}
143+
if (result.peakSessionsOverall > MAX_CONNECTIONS) {
144+
failures.push(`pool exceeded maxConnections: peak=${result.peakSessionsOverall} > ${MAX_CONNECTIONS}`);
145+
}
146+
147+
if (failures.length) {
148+
for (const f of failures) console.error('FAIL:', f);
149+
process.exit(1);
150+
}
151+
console.log('all invariants held');
113152
}
114153

115154
main().catch((err) => {

‎loadtest/h2load.sh‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,21 @@ LOG="$(mktemp)"
2222
PORT=""
2323

2424
cleanup() {
25-
if [[ -n "${SERVER_PID:-}" ]]; then kill "$SERVER_PID" 2>/dev/null || true; fi
25+
if [[ -n "${SERVER_PGID:-}" ]]; then
26+
# Kill the entire process group so npx + the spawned node process both die.
27+
kill -TERM -- "-${SERVER_PGID}" 2>/dev/null || true
28+
sleep 0.2
29+
kill -KILL -- "-${SERVER_PGID}" 2>/dev/null || true
30+
fi
2631
rm -f "$LOG"
2732
}
28-
trap cleanup EXIT
33+
trap cleanup EXIT INT TERM
2934

30-
( cd "$ROOT" && npx tsx loadtest/h2-multiplex-server.ts ) > "$LOG" 2>&1 &
31-
SERVER_PID=$!
35+
# Run the server in its own process group via setsid so we can signal the whole
36+
# group on cleanup. exec replaces the subshell so the PID we capture *is* setsid.
37+
setsid bash -c "cd \"$ROOT\" && exec npx tsx loadtest/h2-multiplex-server.ts" \
38+
> "$LOG" 2>&1 &
39+
SERVER_PGID=$!
3240

3341
for _ in $(seq 1 50); do
3442
if grep -q "listening on port" "$LOG" 2>/dev/null; then
@@ -45,4 +53,5 @@ if [[ -z "$PORT" ]]; then
4553
fi
4654

4755
echo "h2load against https://localhost:$PORT/ (N=$N, C=$C, M=$M)"
48-
h2load -n "$N" -c "$C" -m "$M" "https://localhost:$PORT/"
56+
# -k: skip TLS verification (server uses an ephemeral self-signed cert)
57+
h2load -k -n "$N" -c "$C" -m "$M" "https://localhost:$PORT/"

‎tests/lib/h2-transport/helpers/certs.ts‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { execSync } from 'node:child_process';
1+
import { execFileSync } from 'node:child_process';
22
import fs from 'node:fs';
33
import os from 'node:os';
44
import path from 'node:path';
@@ -8,16 +8,25 @@ let cached: { key: Buffer; cert: Buffer; tmpDir: string } | null = null;
88
/**
99
* Generate (once per process) a self-signed cert for localhost and return the
1010
* key/cert buffers. Cleanup happens via cleanupCerts() in a global afterAll.
11+
*
12+
* Uses execFileSync (argv array, no shell) so paths containing spaces or
13+
* shell metacharacters — common on Windows/macOS user dirs — don't break.
1114
*/
1215
export function ensureCerts(): { key: Buffer; cert: Buffer } {
1316
if (cached) return { key: cached.key, cert: cached.cert };
1417

1518
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'h2-test-'));
1619
const keyPath = path.join(tmpDir, 'key.pem');
1720
const certPath = path.join(tmpDir, 'cert.pem');
18-
execSync(
19-
`openssl req -x509 -newkey rsa:2048 -keyout ${keyPath} -out ${certPath} ` +
20-
`-days 1 -nodes -subj "/CN=localhost" 2>/dev/null`,
21+
execFileSync(
22+
'openssl',
23+
[
24+
'req', '-x509', '-newkey', 'rsa:2048',
25+
'-keyout', keyPath, '-out', certPath,
26+
'-days', '1', '-nodes',
27+
'-subj', '/CN=localhost',
28+
],
29+
{ stdio: ['ignore', 'ignore', 'ignore'] },
2130
);
2231
cached = {
2332
key: fs.readFileSync(keyPath),

0 commit comments

Comments
 (0)