Conversation
StatusReproduced with a mock Postgres server that returns large-field # before fix (debug ASAN, src/ reverted to main)
bun bd test test/js/sql/sql-postgres-error-response-leak.test.ts
# Expected: < 250 Received: 575.62
# after fix
bun bd test test/js/sql/sql-postgres-error-response-leak.test.ts
# passPR: #32285 Rebased onto main (conflict with #33909's per-crate error enums resolved by keeping main's CI: build 71962 finished 283/287 passed. The new
Ready for review. |
WalkthroughChangesPostgres protocol response fields and unrecognized options now use Postgres string ownership
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 2:23 PM PT - Jul 11th, 2026
❌ @robobun, your commit 16a8f80 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 32285That installs a local version of the PR into your bun-32285 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal, correct leak fix mirroring the Zig FieldMessage.deinit(); the two inline notes are non-blocking nits.
Extended reasoning...
Overview
This PR adds a 6-line impl Drop for FieldMessage in src/sql/postgres/protocol/FieldMessage.rs that calls self.payload().deref() to release the +1 WTFStringImpl ref taken by String::clone_utf8 in init(). It also adds a mock-server regression test that pumps ~450 MiB of error/notice field strings through the connection and asserts RSS stays bounded.
I verified the only consumers of FieldMessage: ErrorResponse/NoticeResponse hold a Vec<FieldMessage> with no manual cleanup, and error_response_jsc::to_js only borrows &ErrorResponse and binds &String references — nothing else derefs or takes ownership of the inner strings, so there is no double-free risk. String::deref() is a no-op for non-WTFStringImpl tags, so it's safe across all variants. The change is a direct port of the existing Zig FieldMessage.deinit().
Security risks
None. This is a refcount release on drop in protocol-decode error metadata; no auth, crypto, parsing, or user-controlled control flow is changed.
Level of scrutiny
Low-to-moderate. The production change is tiny, mechanical, and mirrors an established pattern in the Zig reference implementation. The leak is real and the fix is the obvious one. The regression test is thorough (mock server, warm-up, GC sweeps, ASAN-aware threshold) and the PR includes before/after measurements showing ~460 MiB → bounded.
Other factors
Both inline findings are explicitly nits: (1) OwnedString would be the more idiomatic wrapper but requires touching all 18 variants and the current Drop is functionally equivalent; (2) expect(stderr).toBe("") is a documented flake-risk pattern under ASAN but doesn't affect correctness of the fix. Neither blocks merging. No CODEOWNERS cover this path and there are no outstanding human review comments.
There was a problem hiding this comment.
LGTM — both nits addressed in 49f70df; OwnedString is the right pattern here and the consumer in error_response_jsc.rs keeps working unchanged via deref coercion.
Extended reasoning...
Overview
Two files: src/sql/postgres/protocol/FieldMessage.rs swaps the 18 enum variant payloads from bun_core::String (a Copy FFI type with no Drop) to bun_core::OwnedString (the canonical RAII wrapper that deref()s on drop), and init() wraps each String::clone_utf8(...) in OwnedString::new(...) via a small closure. A new mock-server leak test exercises 300 iterations of large ErrorResponse/NoticeResponse payloads and asserts bounded RSS growth.
I left two nits on the first revision (use OwnedString instead of a hand-rolled Drop; drop the strict expect(stderr).toBe("")). Both were applied in 49f70df and the inline threads are resolved.
Security risks
None. This only changes ownership/lifetime of server-provided error-message strings that were already being decoded and surfaced to JS. No auth, crypto, parsing-boundary, or permission changes.
Level of scrutiny
Low-to-moderate. The change is mechanical and now uses the documented in-tree helper exactly as bun_core::OwnedString's doc comment prescribes for clone* constructors. I checked the only external consumer that destructures variants — error_response_jsc::to_js — and it binds by reference into &String locals, which continues to work via OwnedString: Deref<Target = String> coercion with no risk of double-deref (nothing moves or copies the payload out). ErrorResponse.messages: Vec<FieldMessage> dropping now releases each ref automatically. The s closure is only invoked inside matched arms, so the unknown-tag _ => Err path allocates nothing.
Other factors
- Before/after RSS numbers in the description (+461 MiB → bounded) are a clear demonstration the leak is fixed.
- Test follows the existing mock-postgres-server pattern (
postgres-tls-ctx-leaket al.), is ASAN-aware, and no longer asserts exact-empty stderr. - No CODEOWNERS for this path; bug-hunting pass found nothing on the current revision.
There was a problem hiding this comment.
LGTM — all four review nits addressed, clean OwnedString RAII fix with a regression test.
Extended reasoning...
Overview
This PR fixes a memory leak in the Postgres protocol layer where FieldMessage enum variants stored bun_core::String (a Copy FFI handle with no Drop) holding +1 WTFStringImpl refs from String::clone_utf8, leaking 6–12 string allocations per server ErrorResponse/NoticeResponse. The fix swaps the payload type to bun_core::OwnedString (the repo's canonical RAII wrapper) in FieldMessage.rs and applies the same pattern to the latent-but-identical case in NegotiateProtocolVersion.rs. A new mock-server leak test asserts bounded RSS growth across 300 large-field error responses.
Review history
I left four inline comments across two prior review rounds; all four were addressed and resolved:
- Use
OwnedStringinstead of a hand-rolledDropimpl → done in 49f70df - Drop the strict
expect(stderr).toBe("")assertion (ASAN flake risk) → done in 49f70df - Widen the ASAN RSS threshold from 200 to 250 MiB for ~2x headroom → done in 9770715
- Apply the same
OwnedStringfix toNegotiateProtocolVersion.unrecognized_options→ done in 9770715
The bug-hunting pass on the final revision found nothing.
Security risks
None. This is purely internal refcount management on protocol-decoded strings; no parsing logic, auth, crypto, or user-facing API surface changes. The only behavioral change is that strings are now released on drop instead of leaking.
Level of scrutiny
Low–medium. The change is mechanical (String → OwnedString across 18 enum variants plus one Vec field), uses the documented in-tree helper exactly as its doc comment prescribes, and mirrors the Zig reference's deinit(). I verified the sole external consumer (error_response_jsc::to_js) only borrows &FieldMessage and binds the payloads as &OwnedString, which deref-coerces to the existing &String locals — no ownership transfer, no double-free risk. payload() -> &String likewise deref-coerces at the return site. NegotiateProtocolVersion.unrecognized_options has no readers anywhere in src/.
Other factors
The regression test follows repo conventions (subprocess fixture via tempDir/bunExe/bunEnv, ASAN-aware threshold with ~2x headroom, no exact-stderr assertion, combined failure message surfacing stdout/stderr/exitCode). Before/after measurements in the PR description (+461 MiB → bounded in release; +570 → +124 MiB under ASAN) cleanly demonstrate the fix. CI build #62483 is running on the final commit.
FieldMessage stores bun_core::String payloads created via String::clone_utf8 (refcount 1). bun_core::String is Copy and has no Drop, so when Vec<FieldMessage> inside ErrorResponse/NoticeResponse was dropped the WTFStringImpl refs were never released. Every Postgres server error and NOTICE leaked its severity/code/message/detail/hint/ position/where/schema/table/column/file/line/routine strings for the life of the process. The Zig reference (FieldMessage.zig) has an explicit deinit() that derefs the payload; the Rust port had no equivalent. Add impl Drop for FieldMessage that derefs the payload, matching the Zig semantics. Includes a mock-server leak test that fails with ~460-570 MiB RSS growth before the fix and passes after.
Switch FieldMessage variants from bun_core::String (Copy, no Drop) to bun_core::OwnedString so the +1 ref from String::clone_utf8 is released via the canonical RAII wrapper instead of a hand-rolled Drop impl. payload() keeps returning &String via Deref coercion so callers are unchanged. Also stop asserting stderr is exactly empty in the leak test; surface stderr only if the fixture fails to produce JSON output.
NegotiateProtocolVersion.unrecognized_options had the same Vec<bun_core::String> pattern as FieldMessage: clone_utf8 returns a +1 ref that was never released on drop. The struct is currently never decoded (no 'v' handler is wired up) so the leak was latent, but apply the same OwnedString fix so it does not bite when the handler is added. Also remove the comment that incorrectly claimed dropping the Vec releases the refs. Widen the ASAN RSS threshold in the leak test from 200 to 250 MiB for ~2x headroom over the measured +124 MiB; the unfixed build grows by ~570 MiB under ASAN so the regression signal is preserved.
9770715 to
fdc5212
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: 2
🤖 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 `@test/js/sql/sql-postgres-error-response-leak.test.ts`:
- Around line 1-11: Condense the header comments in the Postgres error-response
leak test to no more than three lines total, retaining only the essential test
purpose and behavior. Remove or relocate the detailed rationale about String
ownership, FieldMessage, and RSS validation rather than expanding comments in
the test.
- Around line 106-117: Update the warm-up and RSS iteration loops in this test
to capture each rejected query instead of swallowing errors, and assert the
expected Postgres 42P01 ErrorResponse shape before measuring or evaluating
memory usage. Keep the existing warm-up, iteration count, garbage collection,
and RSS measurement flow unchanged after validating the error.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f22df8b5-9b8c-4a19-8310-f2a6d4f9bd47
📒 Files selected for processing (3)
src/sql/postgres/protocol/FieldMessage.rssrc/sql/postgres/protocol/NegotiateProtocolVersion.rstest/js/sql/sql-postgres-error-response-leak.test.ts
Capture the warm-up error's errno/code and count 42P01 rejections in the loop, then assert them before the RSS threshold. Without this the test could pass silently if the decoder ever stopped rejecting. Also condense the file-header comment to three lines.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/js/sql/sql-postgres-error-response-leak.test.ts`:
- Around line 106-108: Pin the leak-test workload to the required 300 queries:
in the fixture around ITERATIONS, assert that result.ITERATIONS equals 300, and
update the errorCount assertion to compare against the fixed value 300 rather
than result.ITERATIONS. Apply the same change to the corresponding assertion
block noted in the comment.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 55ac1aa4-6979-4848-9d5b-8155cc653dde
📒 Files selected for processing (1)
test/js/sql/sql-postgres-error-response-leak.test.ts
errorCount was compared against result.ITERATIONS, which is self-referential: if the fixture ran fewer iterations both would shrink together and the assertion would still pass. Compare both against the literal 300 so the workload size is pinned independently of the fixture.
Rewrite the ErrorResponse/NoticeResponse leak test to use the shared Postgres wire-protocol helpers in test/js/sql/wire-frames.ts instead of inlining Buffer.alloc frame construction, matching the documented convention in the sibling fault-injection tests. The mock server now runs in the parent test process and only the SQL client whose RSS is measured runs in the subprocess. Add pgNoticeResponse to wire-frames.ts (same field-list body as ErrorResponse, type 'N') and refactor pgErrorResponse to share the body builder via pgRaw; wire-frames.test.ts confirms the bytes are unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/js/sql/sql-postgres-error-response-leak.test.ts`:
- Around line 20-31: Update the listeningServer socket handler to accumulate
incoming TCP data and parse complete PostgreSQL frames with
pgReadFrontendMessages before dispatching. Replace the startup flag and raw
data[0] check with message-based handling, sending authentication/ready
responses for startup messages and queryReply only for parsed query messages,
while retaining partial buffered data until a complete frame is available.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 02197030-c08d-4b79-a27d-972910874a4b
📒 Files selected for processing (2)
test/js/sql/sql-postgres-error-response-leak.test.tstest/js/sql/wire-frames.ts
There was a problem hiding this comment.
LGTM — all prior feedback addressed; the OwnedString substitution is the canonical pattern for clone_utf8 results.
What was reviewed:
payload() -> &Stringand theerror_response_jsc.rsmatch arms still compile viaOwnedString: Deref<Target=String>coercion; consumers only borrow, so no double-free path.pgErrorResponserefactor is byte-identical to the old hand-built frame (pgRawwritestype + Int32(4+body.len) + body, matching the oldlen = 4 + Σfields + 1); existing importers (wire-frames.test.ts,sql-connect-error-reporting.test.ts) unaffected.#[derive(Default)]onNegotiateProtocolVersionstill holds sinceVec<OwnedString>: Defaultregardless of elementDefault.
Extended reasoning...
Overview
Fixes a per-ErrorResponse/NoticeResponse WTFStringImpl leak in the Postgres protocol layer by changing FieldMessage variants and NegotiateProtocolVersion.unrecognized_options from bun_core::String (Copy, no Drop) to bun_core::OwnedString (RAII deref() on drop). Adds a leak regression test and a small wire-frames.ts refactor to share the ErrorResponse/NoticeResponse field-list encoder.
Security risks
None. No user-controlled input handling changes; this only adjusts ownership semantics of already-parsed field strings. OwnedString is move-only, so the compiler statically prevents the double-deref footgun that the old Copy payload allowed.
Level of scrutiny
Medium — native refcounted memory in a runtime data path — but the change is mechanical: a type substitution applying the exact pattern OwnedString's own doc comment prescribes for clone* constructors, already used in 40+ files including the adjacent PostgresSQLConnection.rs/PostgresRequest.rs. I verified the two consumers (error_response_jsc::to_js and ErrorResponse::fmt) only borrow &String via deref coercion and never take ownership, so drop still runs exactly once on the Vec<FieldMessage>. The wire-frames.ts refactor produces identical bytes (checked the length arithmetic; wire-frames.test.ts also asserts it).
Other factors
This PR has been through 7 iterations. All five of my prior inline comments (manual Drop → OwnedString, stderr assertion, ASAN threshold headroom, NegotiateProtocolVersion sibling leak, wire-frames.ts reuse) and all four CodeRabbit comments are resolved in the current HEAD (16a8f80). The evidence block demonstrates fails-without-fix (+578 MiB ASAN) / passes-with-fix (+124 MiB ASAN, bounded release) on both build profiles, and the test now pins ITERATIONS=300 and asserts the 42P01/ERR_POSTGRES_SERVER_ERROR shape before the RSS check so it can't pass vacuously. The bug-hunting system found nothing this run.
|
This leak was reported again independently; it still reproduces on main at 9a543cc ( The branch currently conflicts with main:
The |
|
Closing: #40238 made Verified: the leak test from this branch ( |
1 participant
Problem
Postgres
FieldMessageleaks oneWTFStringImplper field on every serverErrorResponseandNoticeResponse. A typical constraint violation or syntax error carries 6-12 fields (severity, code, message, detail, hint, position, where, schema, table, column, file, line, routine), so each error leaks that many string allocations for the life of the process.Cause
FieldMessage::init()(src/sql/postgres/protocol/FieldMessage.rs) storesbun_core::String::clone_utf8(...)in each enum variant.clone_utf8returns aWTFStringImplwith refcount 1 that the caller must release, butbun_core::Stringis#[derive(Copy)]with noDrop. The Zig reference hasFieldMessage.deinit()callingmessage.deref(); the Rust port had no equivalent, so droppingErrorResponse.messages: Vec<FieldMessage>freed theVecbut never released the inner refs.error_response_jsc::to_jsonly borrows&ErrorResponseand theNoticeResponsehandler decodes-and-drops immediately, so neither consumer releases the refs either.Fix
Add
impl Drop for FieldMessagethat derefs the payload string, mirroring the Zigdeinit().Verification
New
test/js/sql/sql-postgres-error-response-leak.test.tsruns 300 queries against a mock server that replies to each with aNoticeResponse+ErrorResponsecarrying ~1.5 MiB of field strings, then checks RSS growth.Existing postgres mock-server tests (
postgres-multi-statement-fields,postgres-tls-ctx-leak,postgres-binary-*,sql-connect-error-reporting) all pass.[review] gate passed · iteration 7 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 7
evidence per changed file
root cause · written by the author bot
The Rust port of FieldMessage stored each field payload as bun_core::String, a Copy type with no Drop, so the +1 WTFStringImpl reference taken by clone_utf8 during parsing was never released when the containing Vec was dropped. This leaked every severity, code, message, detail, hint, and related string on each Postgres ErrorResponse and NoticeResponse. The fix changes the payload type to OwnedString, the RAII wrapper that derefs on drop, so the strings are released when the message vector is destroyed.