feat(server): WS load-test harness, e2e suite, and Bun runtime support - #4977
Closed
evanpelle wants to merge 1 commit into
Closed
feat(server): WS load-test harness, e2e suite, and Bun runtime support#4977evanpelle wants to merge 1 commit into
evanpelle wants to merge 1 commit into
Conversation
Adds a WebSocket load-test harness (npm run perf:server) that drives the real server (create games, join clients, start, intent traffic) and measures intent->turn-broadcast RTT, tick jitter, and server CPU/RSS, with a selectable server runtime (--runtime node|bun). Makes the server run correctly under Bun and fixes two real lifecycle gaps found doing so: - cluster workers now exit when the master's IPC channel closes; under Bun they otherwise outlive a dead master and keep serving stale state on their SO_REUSEPORT-bound ports (no-op on Node, verified) - WorkerLobbyService.sendToMaster no longer throws on a closed IPC channel (Bun throws where Node returns false) Adds an e2e suite (npm run test:e2e / test:e2e:bun) that boots the real master+workers and exercises the full flow over HTTP+WS: create, join, start, 100ms turn relay, server-stamped intents, rejoin catch-up, kick, wrong-worker rejection, and a master-death worker-exit regression test. Load results (tests/load/README.md): latency identical on both runtimes (tick-dominated), but Bun burns ~4-6x CPU for the same relay workload — transport benchmarks show a Bun.serve()-native rewrite would not close the gap. Recommendation: keep Node as the production runtime; Bun scripts (start:server:bun, dev:bun) stay for experimentation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
WalkthroughAdded Bun runtime commands, worker shutdown handling, dedicated E2E infrastructure, comprehensive game-server E2E coverage, and a configurable WebSocket load-test harness with metrics and result output. ChangesServer validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
tests/e2e/WorkerLifecycle.test.ts (1)
44-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting the shutdown reason, not only the exit.
The test proves the workers are gone. It does not prove they left through the new
disconnectpath insrc/server/Worker.ts. A worker that crashes for an unrelated reason also passes.TestServeralready captures the server output, so you can check for the log line that the new handler writes.♻️ Proposed addition
await waitFor( () => workerPids.every((pid) => !pidAlive(pid)), 10_000, `workers ${workerPids.join(",")} to exit after master SIGKILL`, ); + + // The workers must leave through the IPC-disconnect path, not by crashing. + const shutdownLogs = server.logs.filter((l) => + l.includes("IPC channel to master closed"), + ); + expect(shutdownLogs.length).toBeGreaterThanOrEqual(workerPids.length); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/WorkerLifecycle.test.ts` around lines 44 - 48, Extend the worker shutdown assertion in WorkerLifecycle.test.ts to verify TestServer’s captured output contains the disconnect-handler log emitted by Worker.ts, in addition to confirming all worker PIDs have exited. Assert the expected log entry for each relevant worker so unrelated crashes cannot satisfy the test.vite.config.ts (1)
256-258: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep Vitest’s default excludes and add
tests/e2e/**.Import
configDefaultsfromvitest/configand use[...configDefaults.exclude, "tests/e2e/**"]. The current array removes Vitest’s defaults, including.gitand.cache.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vite.config.ts` around lines 256 - 258, Update the Vitest configuration’s exclude option near the E2E test settings to import and reuse configDefaults from vitest/config, preserving Vitest’s default exclusions while adding tests/e2e/**. Replace the custom exclusion list rather than duplicating or removing the existing defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/GameFlow.test.ts`:
- Around line 113-120: Remove the upper-bound assertion on gained in the “server
broadcasts turns at the 100ms tick” test; retain the lower-bound check that
verifies broadcasting continues, without constraining scheduler timing from
sleep(1200).
In `@tests/e2e/util.ts`:
- Around line 106-123: Update the output-handling logic that populates logs so
stdout and stderr chunks are buffered across writes, split into complete lines,
and only complete lines are stored. Ensure workerPids() and masterPid() continue
matching against reconstructed lines, including when a log message is divided
across chunk boundaries, while retaining any incomplete trailing fragment for
the next chunk.
- Around line 79-103: Update the server startup flow around the spawned process
and health wait to attach an error listener, track early exit or spawn failure,
and stop polling when the process is no longer running. When startup fails,
include the captured stdout/stderr logs in the reported error so missing
binaries and early server crashes are diagnosable; preserve the existing
successful health-check behavior.
- Around line 134-153: Update the stop() cleanup flow around the existing
waitFor call so that after the SIGKILL fallback in its catch handler, it waits
again for all ports from MASTER_PORT through workerPort(NUM_WORKERS - 1) to stop
serving HTTP before returning. Preserve the current immediate cleanup for
already-released ports and keep the final this.proc reset after the post-kill
verification.
In `@tests/load/loadtest.mjs`:
- Around line 583-590: Reset or snapshot the throughput counters msgsReceived,
bytesReceived, and intentsSent immediately before setting metrics.measuring =
true, after the warmup sleep. Ensure the measurement window and subsequent
DURATION_S-based calculations use only traffic generated during the measured
interval.
- Around line 156-159: Update the SIGINT shutdown handler around stopServer to
handle both SIGINT and SIGTERM, ensuring either signal stops the detached server
before exiting with status 130.
---
Nitpick comments:
In `@tests/e2e/WorkerLifecycle.test.ts`:
- Around line 44-48: Extend the worker shutdown assertion in
WorkerLifecycle.test.ts to verify TestServer’s captured output contains the
disconnect-handler log emitted by Worker.ts, in addition to confirming all
worker PIDs have exited. Assert the expected log entry for each relevant worker
so unrelated crashes cannot satisfy the test.
In `@vite.config.ts`:
- Around line 256-258: Update the Vitest configuration’s exclude option near the
E2E test settings to import and reuse configDefaults from vitest/config,
preserving Vitest’s default exclusions while adding tests/e2e/**. Replace the
custom exclusion list rather than duplicating or removing the existing defaults.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b309cccb-7ff1-4dd8-9eaa-b3907213654d
📒 Files selected for processing (13)
.gitignoreeslint.config.jspackage.jsonsrc/server/Worker.tssrc/server/WorkerLobbyService.tstests/e2e/GameFlow.test.tstests/e2e/WorkerLifecycle.test.tstests/e2e/util.tstests/load/README.mdtests/load/loadtest.mjstsconfig.jsonvite.config.tsvitest.e2e.config.ts
Comment on lines
+113
to
+120
| test("server broadcasts turns at the 100ms tick", async () => { | ||
| const before = creator.turns().length; | ||
| await sleep(1200); | ||
| const after = creator.turns().length; | ||
| const gained = after - before; | ||
| // ~12 expected; allow generous slack for CI jitter. | ||
| expect(gained).toBeGreaterThanOrEqual(8); | ||
| expect(gained).toBeLessThanOrEqual(16); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Drop the upper bound on the turn count.
The lower bound checks the real property: the server keeps broadcasting turns. The upper bound checks how accurately this test process wakes from sleep(1200). Under CI load the sleep can overrun by hundreds of milliseconds while the server keeps ticking, so gained exceeds 16 and the test fails for a reason unrelated to the server. Measure the elapsed time and derive the expectation from it, or assert only the lower bound.
💚 Proposed fix
test("server broadcasts turns at the 100ms tick", async () => {
const before = creator.turns().length;
+ const t0 = Date.now();
await sleep(1200);
+ const elapsed = Date.now() - t0;
const after = creator.turns().length;
const gained = after - before;
- // ~12 expected; allow generous slack for CI jitter.
- expect(gained).toBeGreaterThanOrEqual(8);
- expect(gained).toBeLessThanOrEqual(16);
+ // ~1 turn per 100ms. Scale the bound to the time we actually slept, so a
+ // slow test process does not fail the server.
+ expect(gained).toBeGreaterThanOrEqual(8);
+ expect(gained).toBeLessThanOrEqual(Math.ceil(elapsed / 100) + 4);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("server broadcasts turns at the 100ms tick", async () => { | |
| const before = creator.turns().length; | |
| await sleep(1200); | |
| const after = creator.turns().length; | |
| const gained = after - before; | |
| // ~12 expected; allow generous slack for CI jitter. | |
| expect(gained).toBeGreaterThanOrEqual(8); | |
| expect(gained).toBeLessThanOrEqual(16); | |
| test("server broadcasts turns at the 100ms tick", async () => { | |
| const before = creator.turns().length; | |
| const t0 = Date.now(); | |
| await sleep(1200); | |
| const elapsed = Date.now() - t0; | |
| const after = creator.turns().length; | |
| const gained = after - before; | |
| // ~1 turn per 100ms. Scale the bound to the time we actually slept, so a | |
| // slow test process does not fail the server. | |
| expect(gained).toBeGreaterThanOrEqual(8); | |
| expect(gained).toBeLessThanOrEqual(Math.ceil(elapsed / 100) + 4); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/GameFlow.test.ts` around lines 113 - 120, Remove the upper-bound
assertion on gained in the “server broadcasts turns at the 100ms tick” test;
retain the lower-bound check that verifies broadcasting continues, without
constraining scheduler timing from sleep(1200).
Comment on lines
+79
to
+103
| const [cmd, args] = serverCommand(); | ||
| this.proc = spawn(cmd, args, { | ||
| cwd: repoRoot, | ||
| env: serverEnv, | ||
| detached: true, // own process group so stop() can kill the whole tree | ||
| stdio: ["ignore", "pipe", "pipe"], | ||
| }); | ||
| this.proc.stdout!.on("data", (d) => this.logs.push(String(d))); | ||
| this.proc.stderr!.on("data", (d) => this.logs.push(String(d))); | ||
|
|
||
| await waitFor( | ||
| async () => { | ||
| try { | ||
| const res = await fetch( | ||
| `http://127.0.0.1:${MASTER_PORT}/api/health`, | ||
| { signal: AbortSignal.timeout(1000) }, | ||
| ); | ||
| return res.ok; | ||
| } catch { | ||
| return false; | ||
| } | ||
| }, | ||
| 60_000, | ||
| "server health", | ||
| ); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle spawn failure and early server exit.
spawn emits an error event when the command is missing. bun is an external binary and may not be installed. No error listener is attached, so Node re-throws the event as an unhandled error in the test process. If the server instead starts and then crashes, start() waits the full 60 seconds and fails with "server health" only. The captured logs are never shown, so the cause stays hidden.
Track the exit state and include the logs in the failure.
🛠️ Proposed fix
const [cmd, args] = serverCommand();
this.proc = spawn(cmd, args, {
cwd: repoRoot,
env: serverEnv,
detached: true, // own process group so stop() can kill the whole tree
stdio: ["ignore", "pipe", "pipe"],
});
this.proc.stdout!.on("data", (d) => this.logs.push(String(d)));
this.proc.stderr!.on("data", (d) => this.logs.push(String(d)));
+ let exited: string | null = null;
+ // Without an `error` listener Node throws the event at the test process.
+ this.proc.on("error", (err) => {
+ exited = `failed to spawn ${cmd}: ${err.message}`;
+ });
+ this.proc.on("exit", (code, signal) => {
+ exited ??= `server exited early (code ${code}, signal ${signal})`;
+ });
- await waitFor(
- async () => {
- try {
- const res = await fetch(
- `http://127.0.0.1:${MASTER_PORT}/api/health`,
- { signal: AbortSignal.timeout(1000) },
- );
- return res.ok;
- } catch {
- return false;
- }
- },
- 60_000,
- "server health",
- );
+ try {
+ await waitFor(
+ async () => {
+ if (exited !== null) throw new Error(exited);
+ try {
+ const res = await fetch(
+ `http://127.0.0.1:${MASTER_PORT}/api/health`,
+ { signal: AbortSignal.timeout(1000) },
+ );
+ return res.ok;
+ } catch {
+ return false;
+ }
+ },
+ 60_000,
+ "server health",
+ );
+ } catch (err) {
+ throw new Error(`${err}\n--- server output ---\n${this.logs.join("")}`);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const [cmd, args] = serverCommand(); | |
| this.proc = spawn(cmd, args, { | |
| cwd: repoRoot, | |
| env: serverEnv, | |
| detached: true, // own process group so stop() can kill the whole tree | |
| stdio: ["ignore", "pipe", "pipe"], | |
| }); | |
| this.proc.stdout!.on("data", (d) => this.logs.push(String(d))); | |
| this.proc.stderr!.on("data", (d) => this.logs.push(String(d))); | |
| await waitFor( | |
| async () => { | |
| try { | |
| const res = await fetch( | |
| `http://127.0.0.1:${MASTER_PORT}/api/health`, | |
| { signal: AbortSignal.timeout(1000) }, | |
| ); | |
| return res.ok; | |
| } catch { | |
| return false; | |
| } | |
| }, | |
| 60_000, | |
| "server health", | |
| ); | |
| const [cmd, args] = serverCommand(); | |
| this.proc = spawn(cmd, args, { | |
| cwd: repoRoot, | |
| env: serverEnv, | |
| detached: true, // own process group so stop() can kill the whole tree | |
| stdio: ["ignore", "pipe", "pipe"], | |
| }); | |
| this.proc.stdout!.on("data", (d) => this.logs.push(String(d))); | |
| this.proc.stderr!.on("data", (d) => this.logs.push(String(d))); | |
| let exited: string | null = null; | |
| // Without an `error` listener Node throws the event at the test process. | |
| this.proc.on("error", (err) => { | |
| exited = `failed to spawn ${cmd}: ${err.message}`; | |
| }); | |
| this.proc.on("exit", (code, signal) => { | |
| exited ??= `server exited early (code ${code}, signal ${signal})`; | |
| }); | |
| try { | |
| await waitFor( | |
| async () => { | |
| if (exited !== null) throw new Error(exited); | |
| try { | |
| const res = await fetch( | |
| `http://127.0.0.1:${MASTER_PORT}/api/health`, | |
| { signal: AbortSignal.timeout(1000) }, | |
| ); | |
| return res.ok; | |
| } catch { | |
| return false; | |
| } | |
| }, | |
| 60_000, | |
| "server health", | |
| ); | |
| } catch (err) { | |
| throw new Error(`${err}\n--- server output ---\n${this.logs.join("")}`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/util.ts` around lines 79 - 103, Update the server startup flow
around the spawned process and health wait to attach an error listener, track
early exit or spawn failure, and stop polling when the process is no longer
running. When startup fails, include the captured stdout/stderr logs in the
reported error so missing binaries and early server crashes are diagnosable;
preserve the existing successful health-check behavior.
Comment on lines
+106
to
+123
| // PIDs (from the server's own logs) of the cluster worker processes. | ||
| workerPids(): number[] { | ||
| const pids: number[] = []; | ||
| for (const line of this.logs) { | ||
| for (const m of line.matchAll(/Started worker \d+ \(PID: (\d+)\)/g)) { | ||
| pids.push(Number(m[1])); | ||
| } | ||
| } | ||
| return pids; | ||
| } | ||
|
|
||
| masterPid(): number | null { | ||
| for (const line of this.logs) { | ||
| const m = line.match(/Primary (\d+) is running/); | ||
| if (m) return Number(m[1]); | ||
| } | ||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Split captured output into lines before matching.
Lines 86-87 push raw stdout and stderr chunks into logs. A chunk boundary can cut a log line in two. Started worker 1 (PID: 12345) can then arrive as Started worker 1 (PI plus D: 12345), and neither part matches the regex. WorkerLifecycle.test.ts asserts workerPids.length is at least 2, so this makes that test flaky.
Buffer the output and store complete lines.
🛠️ Proposed fix
export class TestServer {
proc: ChildProcess | null = null;
logs: string[] = [];
+ private pending = "";
+
+ // stdout arrives in chunks that can cut a log line in half, so keep the
+ // tail until its newline shows up.
+ private capture(chunk: string): void {
+ this.pending += chunk;
+ const lines = this.pending.split("\n");
+ this.pending = lines.pop() ?? "";
+ this.logs.push(...lines);
+ }- this.proc.stdout!.on("data", (d) => this.logs.push(String(d)));
- this.proc.stderr!.on("data", (d) => this.logs.push(String(d)));
+ this.proc.stdout!.on("data", (d) => this.capture(String(d)));
+ this.proc.stderr!.on("data", (d) => this.capture(String(d)));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/util.ts` around lines 106 - 123, Update the output-handling logic
that populates logs so stdout and stderr chunks are buffered across writes,
split into complete lines, and only complete lines are stored. Ensure
workerPids() and masterPid() continue matching against reconstructed lines,
including when a log message is divided across chunk boundaries, while retaining
any incomplete trailing fragment for the next chunk.
Comment on lines
+134
to
+153
| await waitFor( | ||
| async () => { | ||
| for (let p = MASTER_PORT; p <= workerPort(NUM_WORKERS - 1); p++) { | ||
| if (await portServesHttp(p)) return false; | ||
| } | ||
| return true; | ||
| }, | ||
| 15_000, | ||
| "server ports released", | ||
| ).catch(() => { | ||
| // Last resort: SIGKILL the group. | ||
| if (this.proc?.pid) { | ||
| try { | ||
| process.kill(-this.proc.pid, "SIGKILL"); | ||
| } catch { | ||
| // group already gone | ||
| } | ||
| } | ||
| }); | ||
| this.proc = null; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Wait for the ports again after the SIGKILL fallback.
The catch handler sends SIGKILL and then stop() returns at once. It never confirms that the ports are free. fileParallelism is false, so the next suite starts right after. Its start() pre-flight can still see port 3000 serving HTTP and throw "kill strays first", which fails an unrelated suite.
Wait for release after the SIGKILL.
🛠️ Proposed fix
+ private portsFree = async (): Promise<boolean> => {
+ for (let p = MASTER_PORT; p <= workerPort(NUM_WORKERS - 1); p++) {
+ if (await portServesHttp(p)) return false;
+ }
+ return true;
+ };
+
async stop(): Promise<void> {
if (this.proc?.pid) {
try {
process.kill(-this.proc.pid, "SIGTERM");
} catch {
// already dead
}
}
// Wait until every port is released so the next suite can bind.
- await waitFor(
- async () => {
- for (let p = MASTER_PORT; p <= workerPort(NUM_WORKERS - 1); p++) {
- if (await portServesHttp(p)) return false;
- }
- return true;
- },
- 15_000,
- "server ports released",
- ).catch(() => {
- // Last resort: SIGKILL the group.
- if (this.proc?.pid) {
- try {
- process.kill(-this.proc.pid, "SIGKILL");
- } catch {
- // group already gone
- }
- }
- });
+ try {
+ await waitFor(this.portsFree, 15_000, "server ports released");
+ } catch {
+ // Last resort: SIGKILL the group, then confirm the ports really go.
+ if (this.proc?.pid) {
+ try {
+ process.kill(-this.proc.pid, "SIGKILL");
+ } catch {
+ // group already gone
+ }
+ }
+ await waitFor(this.portsFree, 10_000, "server ports released after kill");
+ }
this.proc = null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await waitFor( | |
| async () => { | |
| for (let p = MASTER_PORT; p <= workerPort(NUM_WORKERS - 1); p++) { | |
| if (await portServesHttp(p)) return false; | |
| } | |
| return true; | |
| }, | |
| 15_000, | |
| "server ports released", | |
| ).catch(() => { | |
| // Last resort: SIGKILL the group. | |
| if (this.proc?.pid) { | |
| try { | |
| process.kill(-this.proc.pid, "SIGKILL"); | |
| } catch { | |
| // group already gone | |
| } | |
| } | |
| }); | |
| this.proc = null; | |
| private portsFree = async (): Promise<boolean> => { | |
| for (let p = MASTER_PORT; p <= workerPort(NUM_WORKERS - 1); p++) { | |
| if (await portServesHttp(p)) return false; | |
| } | |
| return true; | |
| }; | |
| async stop(): Promise<void> { | |
| if (this.proc?.pid) { | |
| try { | |
| process.kill(-this.proc.pid, "SIGTERM"); | |
| } catch { | |
| // already dead | |
| } | |
| } | |
| // Wait until every port is released so the next suite can bind. | |
| try { | |
| await waitFor(this.portsFree, 15_000, "server ports released"); | |
| } catch { | |
| // Last resort: SIGKILL the group, then confirm the ports really go. | |
| if (this.proc?.pid) { | |
| try { | |
| process.kill(-this.proc.pid, "SIGKILL"); | |
| } catch { | |
| // group already gone | |
| } | |
| } | |
| await waitFor(this.portsFree, 10_000, "server ports released after kill"); | |
| } | |
| this.proc = null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/util.ts` around lines 134 - 153, Update the stop() cleanup flow
around the existing waitFor call so that after the SIGKILL fallback in its catch
handler, it waits again for all ports from MASTER_PORT through
workerPort(NUM_WORKERS - 1) to stop serving HTTP before returning. Preserve the
current immediate cleanup for already-released ports and keep the final
this.proc reset after the post-kill verification.
Comment on lines
+156
to
+159
| process.on("SIGINT", () => { | ||
| stopServer(); | ||
| process.exit(130); | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'detached: true|process\.on\("SIG(INT|TERM)"|function stopServer' tests/load/loadtest.mjsRepository: openfrontio/OpenFrontIO
Length of output: 842
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '100,175p' tests/load/loadtest.mjs
node - <<'JS'
const { spawn } = require("node:child_process");
const fs = require("node:fs");
const path = require("node:path");
const file = path.join("/tmp", "signal-handler-probe.mjs");
fs.writeFileSync(file, `
process.on("SIGINT", () => process.stdout.write("SIGINT handled\\n"));
setInterval(() => {}, 1000);
`);
const child = spawn(process.execPath, [file], { stdio: ["ignore", "pipe", "inherit"] });
let output = "";
child.stdout.on("data", data => { output += data; });
setTimeout(() => child.kill("SIGTERM"), 100);
child.on("exit", (code, signal) => {
console.log(JSON.stringify({ code, signal, output }));
});
JSRepository: openfrontio/OpenFrontIO
Length of output: 2348
Stop the detached server on SIGTERM.
When CI sends SIGTERM to the harness, the SIGINT handler does not run. The detached server process group can remain alive and retain its ports. Use one shutdown handler for both signals.
Proposed fix
-process.on("SIGINT", () => {
+function exitWithServerStopped(code) {
stopServer();
- process.exit(130);
-});
+ process.exit(code);
+}
+
+process.on("SIGINT", () => exitWithServerStopped(130));
+process.on("SIGTERM", () => exitWithServerStopped(143));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| process.on("SIGINT", () => { | |
| stopServer(); | |
| process.exit(130); | |
| }); | |
| function exitWithServerStopped(code) { | |
| stopServer(); | |
| process.exit(code); | |
| } | |
| process.on("SIGINT", () => exitWithServerStopped(130)); | |
| process.on("SIGTERM", () => exitWithServerStopped(143)); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/load/loadtest.mjs` around lines 156 - 159, Update the SIGINT shutdown
handler around stopServer to handle both SIGINT and SIGTERM, ensuring either
signal stops the detached server before exiting with status 130.
Comment on lines
+583
to
+590
| for (const g of games) for (const c of g.clients) c.startTraffic(); | ||
| await sleep(5000); // warmup: JIT, connection settling | ||
|
|
||
| console.log(`measuring for ${DURATION_S}s...`); | ||
| const windowStart = Date.now(); | ||
| metrics.measuring = true; | ||
| await sleep(DURATION_S * 1000); | ||
| metrics.measuring = false; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Start throughput counters at the measurement boundary.
Lines 583-584 start traffic before the warmup. msgsReceived, bytesReceived, and intentsSent include warmup traffic, but Lines 604-606 divide them by DURATION_S only. This overstates reported throughput.
Reset or snapshot these counters immediately before metrics.measuring = true.
Proposed fix
console.log(`measuring for ${DURATION_S}s...`);
+ for (const g of games) {
+ for (const c of g.clients) {
+ c.msgsReceived = 0;
+ c.bytesReceived = 0;
+ c.intentsSent = 0;
+ }
+ }
const windowStart = Date.now();
metrics.measuring = true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const g of games) for (const c of g.clients) c.startTraffic(); | |
| await sleep(5000); // warmup: JIT, connection settling | |
| console.log(`measuring for ${DURATION_S}s...`); | |
| const windowStart = Date.now(); | |
| metrics.measuring = true; | |
| await sleep(DURATION_S * 1000); | |
| metrics.measuring = false; | |
| for (const g of games) for (const c of g.clients) c.startTraffic(); | |
| await sleep(5000); // warmup: JIT, connection settling | |
| console.log(`measuring for ${DURATION_S}s...`); | |
| for (const g of games) { | |
| for (const c of g.clients) { | |
| c.msgsReceived = 0; | |
| c.bytesReceived = 0; | |
| c.intentsSent = 0; | |
| } | |
| } | |
| const windowStart = Date.now(); | |
| metrics.measuring = true; | |
| await sleep(DURATION_S * 1000); | |
| metrics.measuring = false; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/load/loadtest.mjs` around lines 583 - 590, Reset or snapshot the
throughput counters msgsReceived, bytesReceived, and intentsSent immediately
before setting metrics.measuring = true, after the warmup sleep. Ensure the
measurement window and subsequent DURATION_S-based calculations use only traffic
generated during the measured interval.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Investigates a Node → Bun migration for the game server, starting from a new WebSocket load-test harness, and lands the harness, an e2e suite, Bun runtime support, and two lifecycle fixes. Recommendation: keep Node as the production runtime — data below.
What's in here
tests/load/— WS load-test harness (npm run perf:server). Boots the real server (selectable runtime via--runtime node|bun), creates private games, connects hundreds of WS clients, and measures intent→turn-broadcast RTT, tick jitter, join latency, throughput, and server CPU/RSS. RTT is measured with zero server instrumentation by embeddingDate.now()in thetroopsfield of relayedattackintents. Methodology + full results intests/load/README.md.tests/e2e/— e2e suite (npm run test:e2e,npm run test:e2e:bun) that boots the real master + cluster workers and drives the full flow over HTTP+WS: create → join → start → 100ms turn relay → server-stamped intent relay → rejoin catch-up → garbage-message kick → wrong-worker rejection, plus a master-death → workers-exit regression test.src/server/Worker.ts— workers exit when the master's IPC channel closes. Under Bun, cluster workers otherwise outlive a dead master and keep serving stale state on their SO_REUSEPORT-bound ports next to a restarted server. No-op on Node (verified by running the regression test without the fix).src/server/WorkerLobbyService.ts—sendToMasterno longer throws on a closed IPC channel (Bun throws where Node returnsfalse).package.json—start:server:bun,start:server-dev:bun,dev:bun,test:e2e,test:e2e:bun,perf:server.Load results (Linux x64, 20 cores, Node 26.5 / Bun 1.3.14)
Heavy profile: 400 clients, 2 intents/s each, ~4,400 msgs/s outbound, 60s.
Latency is identical (tick-dominated), but Bun burns ~4–6× the CPU. Transport microbenchmarks rule out the fixable suspects: Bun's builtin
wsshim (3.8%) ≈ nativeBun.serve()(4.1%) vs Node+ws(2.2%) on a minimal broadcast server — so a Bun-native transport rewrite would not close the gap. Per-op JSON+Zod is faster on Bun in isolation; the overhead is diffuse in-situ runtime cost (timer-context sends, GC pressure, ~5%/process idle floor). CPU profile shows ~38% of worker time inside native websocketsendat ~10× the per-send cost the same shim achieves in the minimal bench.Test plan
vitest run tests/server— 305 passednpm run test:e2e(Node) — 13 passednpm run test:e2e:bun(Bun 1.3.14) — 13 passedNODE_OPTIONS=--no-experimental-webstorageon Node 26, pre-existing issue)tsc --noEmit,npm run lint, prettier clean🤖 Generated with Claude Code