Fix server stability issue with oversized requests #22701
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
robobun
force-pushed
the
claude/fix-oversized-request-stability
branch
from
2498312 to
3670a49
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/api/server.zig (1)
2216-2230: Ensure RequestContext cleanup and standardize 413 reason phrase
Problem: prepareJsRequestContext registers an abort handler then calls resp.endWithoutBody(true) and returns. If onAborted does not run for this server-initiated end, RequestContext cleanup (deref → deinit → server.pending_requests--) may be skipped and leak the request state.
Fix: make cleanup deterministic instead of relying solely on the abort handler. In src/bun.js/api/server.zig (prepareJsRequestContext, oversized-body early-return) replace the current block with something like:
ctx.setAbortHandler();
if (comptime Environment.enable_logs) httplog("rejecting request: content-length={d} > limit={d}", .{ req_len, this.config.max_request_body_size });
resp.writeStatus("413 Payload Too Large");
resp.clearAborted(); // remove the abort callback before we detach/end
ctx.endWithoutBody(true); // does detach + end + deferred deref/deinit
return null;(If you prefer not to call ctx.endWithoutBody, ensure you at minimum clear the abort handler, call resp.endWithoutBody(true) and then explicitly ctx.deref() so deinit runs.)
Minor: use the RFC7231 reason phrase "413 Payload Too Large" for consistency.
Optional: keep the suggested debug httplog for easier tracing.
🧹 Nitpick comments (10)
src/bun.js/api/server.zig (1)
2216-2221: Guard against invalid/overflowing Content-Length values.
parseIntfailure maps to 0, which can bypass the size check. Treat parse errors as “too large” to be conservative.- if (req.header("content-length")) |content_length| { - break :brk std.fmt.parseInt(usize, content_length, 10) catch 0; - } + if (req.header("content-length")) |content_length| { + break :brk std.fmt.parseInt(usize, content_length, 10) catch this.config.max_request_body_size + 1; + }test/regression/issue/22353.test.ts (9)
34-41: Remove noop interval and SIGSEGV handler.They don’t strengthen the test; JS cannot reliably trap SIGSEGV, and the server keeps the process alive.
- // Keep process alive - setInterval(() => {}, 1000); - - // Catch segfaults - process.on('SIGSEGV', () => { - console.error('SEGMENTATION FAULT OCCURRED'); - process.exit(139); - });
45-51: Useawait usingfor subprocess lifecycle.Ensures cleanup even if the test throws.
- const proc = Bun.spawn({ + await using proc = Bun.spawn({ cmd: [bunExe(), serverFile], env: bunEnv, stdout: "pipe", stderr: "pipe", });Confirm your Bun version in CI supports
await usingforSubprocess.
63-63: Prefer Buffer alloc over string repeat for large payloads.Matches repo test guidelines for big/repetitive data.
- const largeBody = "x".repeat(2048); + const largeBody = Buffer.alloc(2048, "x").toString();
66-75: Assert 413 when response exists.Strengthens the regression by validating the exact failure mode.
let largeResponse; let largeError; try { largeResponse = await fetch(`http://localhost:${port}`, { method: "POST", body: largeBody, }); } catch (e: any) { // Expected: server should reject with 413 or close connection largeError = e; } + if (largeResponse) { + expect(largeResponse.status).toBe(413); + }
78-79: Avoid arbitrary sleeps; race normal request against process exit.Reduces flakiness per testing guidelines.
- // Small delay to ensure server processes the rejection - await Bun.sleep(500); - - // Send normal request - this should work if server didn't segfault - let normalResponse; - let normalError; - try { - normalResponse = await fetch(`http://localhost:${port}`, { - method: "POST", - body: JSON.stringify({ test: "normal" }), - }); - } catch (e: any) { - normalError = e; - } - - // Wait for process to potentially exit - await Bun.sleep(100); + // Send normal request - this should work if server didn't segfault + const normalReq = fetch(`http://localhost:${port}`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ test: "normal" }), + }).then( + r => ({ ok: true as const, r }), + e => ({ ok: false as const, e }), + ); + const raced = await Promise.race([normalReq, proc.exited.then(code => ({ exited: true as const, code }))]); + let normalResponse: Response | undefined; + let normalError: any; + if ((raced as any).exited) { + normalError = new Error("process exited"); + } else if ((raced as any).ok) { + normalResponse = (raced as any).r; + } else { + normalError = (raced as any).e; + }Also applies to: 93-94, 80-91
84-87: Send JSON with proper content type.Minor correctness/a11y for intermediaries.
- normalResponse = await fetch(`http://localhost:${port}`, { - method: "POST", - body: JSON.stringify({ test: "normal" }), - }); + normalResponse = await fetch(`http://localhost:${port}`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ test: "normal" }), + });
100-114: Simplify stderr capture; avoid partial reads with timeouts.Read once after exit to avoid truncation.
- let stderrOutput = ""; - try { - const stderrReader = proc.stderr.getReader(); - let done = false; - while (!done) { - const result = await Promise.race([ - stderrReader.read(), - new Promise(resolve => setTimeout(() => resolve({ done: true }), 100)), - ]); - if (result.done) break; - stderrOutput += decoder.decode(result.value); - } - stderrReader.releaseLock(); - } catch {} + let stderrOutput = ""; + if (proc.exitCode !== null) { + stderrOutput = await new Response(proc.stderr!).text(); + }
156-156: Remove explicit per-test timeout.Project guidelines say not to set explicit test timeouts.
-}, 10000); +});
149-152: Cleanup is handled byawait using.If you adopt
await using, this manual block can go.- if (proc.exitCode === null) { - proc.kill(); - await proc.exited; - } + // With `await using proc = Bun.spawn(...)`, the process is disposed automatically.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/bun.js/api/server.zig(1 hunks)test/regression/issue/22353.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/regression/issue/22353.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/regression/issue/22353.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/regression/issue/22353.test.ts
test/regression/issue/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Place regression tests under
test/regression/issue/and organize by issue number
Files:
test/regression/issue/22353.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/regression/issue/22353.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/regression/issue/22353.test.ts
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/bun.js/api/server.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/bun.js/api/server.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/server.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/api/server.zig
🧠 Learnings (7)
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/bundle.test.ts : bundle.test.ts should contain DevServer-specific bundling tests
Applied to files:
test/regression/issue/22353.test.ts
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add a corresponding test case in test/v8/v8.test.ts that invokes checkSameOutput with the new function
Applied to files:
test/regression/issue/22353.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/regression/issue/22353.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/regression/issue/22353.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/regression/issue/22353.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: For Node.js compatibility bugs, validate expected behavior in Node.js first, then confirm failure on Bun canary
Applied to files:
test/regression/issue/22353.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Use `using`/`await using` with Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Applied to files:
test/regression/issue/22353.test.ts
robobun
force-pushed
the
claude/fix-oversized-request-stability
branch
from
a996c00 to
b35526a
Compare
Improves request handling for edge cases. Fixes #22353 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
robobun
force-pushed
the
claude/fix-oversized-request-stability
branch
from
dd6d468 to
7a94b5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/bun.js/api/server.zig (2)
2147-2173: Harden Content-Length parsing and handle overflow before 413Good early short‑circuit. Two improvements:
- Parse as u64 and clamp/guard overflow so extremely large values still trigger 413 on 32‑bit targets instead of falling back to 0.
- Optionally log (debug) the rejection to aid regressions.
Suggested change:
- const request_body_length: ?usize = request_body_length: { + const request_body_length: ?usize = request_body_length: { if ((HTTP.Method.which(req.method()) orelse HTTP.Method.OPTIONS).hasRequestBody()) { - const len: usize = brk: { - if (req.header("content-length")) |content_length| { - break :brk std.fmt.parseInt(usize, content_length, 10) catch 0; - } - - break :brk 0; - }; + const len: usize = brk: { + if (req.header("content-length")) |content_length| { + // Parse as u64 to avoid usize overflow; clamp to usize::max + const parsed = std.fmt.parseInt(u64, content_length, 10) catch std.math.maxInt(u64); + break :brk if (parsed > std.math.maxInt(usize)) std.math.maxInt(usize) else @intCast(parsed); + } + break :brk 0; + }; // Abort the request very early. if (len > this.config.max_request_body_size) { resp.writeStatus("413 Request Entity Too Large"); + if (comptime Environment.isDebug) { + httplog("413 early reject: content-length={d} > max={d} ({s} {s})", .{ len, this.config.max_request_body_size, req.method(), req.url() }); + } resp.endWithoutBody(true); return null; } break :request_body_length len; } break :request_body_length null; };
2225-2245: Detect chunked Transfer-Encoding specifically before enabling body streamingCurrently, any Transfer-Encoding presence sets is_transfer_encoding. Restricting to “chunked” avoids false positives (e.g., identity).
- if (request_body_length) |req_len| { + if (request_body_length) |req_len| { ctx.request_body_content_len = req_len; - ctx.flags.is_transfer_encoding = req.header("transfer-encoding") != null; + const te = req.header("transfer-encoding"); + ctx.flags.is_transfer_encoding = te != null and std.mem.indexOf(u8, te.?, "chunked") != null; if (req_len > 0 or ctx.flags.is_transfer_encoding) { // we defer pre-allocating the body until we receive the first chunk // that way if the client is lying about how big the body is or the client aborts // we don't waste memory ctx.request_body.?.value = .{ .Locked = .{ .task = ctx, .global = this.globalThis, .onStartBuffering = RequestContext.onStartBufferingCallback, .onStartStreaming = RequestContext.onStartStreamingRequestBodyCallback, .onReadableStreamAvailable = RequestContext.onRequestBodyReadableStreamAvailable, }, }; ctx.flags.is_waiting_for_request_body = true; resp.onData(*RequestContext, RequestContext.onBufferedBodyChunk, ctx); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/bun.js/api/server.zig(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/bun.js/api/server.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/bun.js/api/server.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/server.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/api/server.zig
🧠 Learnings (2)
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/bun.js/api/server.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/bun.js/api/server.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/regression/issue/22353.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/regression/issue/22353.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/regression/issue/22353.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/regression/issue/22353.test.ts
test/regression/issue/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Place regression tests under
test/regression/issue/and organize by issue number
Files:
test/regression/issue/22353.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/regression/issue/22353.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/regression/issue/22353.test.ts
🧠 Learnings (8)
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/bundle.test.ts : bundle.test.ts should contain DevServer-specific bundling tests
Applied to files:
test/regression/issue/22353.test.ts
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add a corresponding test case in test/v8/v8.test.ts that invokes checkSameOutput with the new function
Applied to files:
test/regression/issue/22353.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/regression/issue/22353.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/regression/issue/22353.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Use `using`/`await using` with Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Applied to files:
test/regression/issue/22353.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/regression/issue/22353.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Do not set explicit test timeouts; rely on Bun’s built-in timeouts
Applied to files:
test/regression/issue/22353.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/regression/issue/22353.test.ts
| test("issue #22353 - server should handle oversized request without crashing", async () => { | ||
| using server = Bun.serve({ | ||
| port: 0, | ||
| maxRequestBodySize: 1024, // 1KB limit | ||
| async fetch(req) { | ||
| const body = await req.text(); | ||
| return new Response( | ||
| JSON.stringify({ | ||
| received: true, | ||
| size: body.length, | ||
| }), | ||
| { | ||
| headers: { "Content-Type": "application/json" }, | ||
| }, | ||
| ); | ||
| }, | ||
| }); | ||
|
|
||
| const resp = await fetch(server.url, { | ||
| method: "POST", | ||
| body: "A".repeat(1025), | ||
| }); | ||
| expect(resp.status).toBe(413); | ||
| expect(await resp.text()).toBeEmpty(); | ||
| for (let i = 0; i < 100; i++) { | ||
| const resp2 = await fetch(server.url, { | ||
| method: "POST", | ||
| }); | ||
| expect(resp2.status).toBe(200); | ||
| expect(await resp2.json()).toEqual({ | ||
| received: true, | ||
| size: 0, | ||
| }); | ||
| } | ||
| }, 10000); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Drop the explicit timeout argument
Line 37 forces a 10 s cap on the test, which our testing guidelines forbid because it introduces avoidable flakiness. Let Bun’s default timeout handle this instead—nothing in the body requires a custom duration.
-}, 10000);
+});As per coding guidelines
📝 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.
| test("issue #22353 - server should handle oversized request without crashing", async () => { | |
| using server = Bun.serve({ | |
| port: 0, | |
| maxRequestBodySize: 1024, // 1KB limit | |
| async fetch(req) { | |
| const body = await req.text(); | |
| return new Response( | |
| JSON.stringify({ | |
| received: true, | |
| size: body.length, | |
| }), | |
| { | |
| headers: { "Content-Type": "application/json" }, | |
| }, | |
| ); | |
| }, | |
| }); | |
| const resp = await fetch(server.url, { | |
| method: "POST", | |
| body: "A".repeat(1025), | |
| }); | |
| expect(resp.status).toBe(413); | |
| expect(await resp.text()).toBeEmpty(); | |
| for (let i = 0; i < 100; i++) { | |
| const resp2 = await fetch(server.url, { | |
| method: "POST", | |
| }); | |
| expect(resp2.status).toBe(200); | |
| expect(await resp2.json()).toEqual({ | |
| received: true, | |
| size: 0, | |
| }); | |
| } | |
| }, 10000); | |
| test("issue #22353 - server should handle oversized request without crashing", async () => { | |
| using server = Bun.serve({ | |
| port: 0, | |
| maxRequestBodySize: 1024, // 1KB limit | |
| async fetch(req) { | |
| const body = await req.text(); | |
| return new Response( | |
| JSON.stringify({ | |
| received: true, | |
| size: body.length, | |
| }), | |
| { | |
| headers: { "Content-Type": "application/json" }, | |
| }, | |
| ); | |
| }, | |
| }); | |
| const resp = await fetch(server.url, { | |
| method: "POST", | |
| body: "A".repeat(1025), | |
| }); | |
| expect(resp.status).toBe(413); | |
| expect(await resp.text()).toBeEmpty(); | |
| for (let i = 0; i < 100; i++) { | |
| const resp2 = await fetch(server.url, { | |
| method: "POST", | |
| }); | |
| expect(resp2.status).toBe(200); | |
| expect(await resp2.json()).toEqual({ | |
| received: true, | |
| size: 0, | |
| }); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In test/regression/issue/22353.test.ts around lines 3 to 37, the test call
includes an explicit timeout argument (10000) which should be removed per
guidelines; delete the final timeout parameter and the trailing comma so the
test uses the default timeout (i.e., change the test invocation to omit the ",
10000" portion and ensure the closing parentheses/braces remain balanced).
Summary
Improves server stability when handling certain request edge cases.
Test plan
test/regression/issue/22353.test.tsFixes #22353
🤖 Generated with Claude Code