fix(socket): don't unprotect never-protected handlers on socket config validation errors

[robobun]

What does this PR do?

Fixes a crash found by fuzzing (panic: assertion failed: self.protection_count > 0).

Bun.connect() / Bun.listen() with an invalid socket handlers object — e.g. missing both data and drain, or a non-callable callback — aborted debug/assert builds instead of throwing:

Bun.connect({ socket: {} });
// panic: assertion failed: self.protection_count > 0
//   Handlers::unprotect (src/runtime/socket/Handlers.rs:363)
//   <Handlers as Drop>::drop
//   Handlers::from_generated
//   SocketConfig::from_generated
//   Listener::connect_inner

Handlers::from_generated constructed the Handlers value first and validated the callbacks afterwards, returning Err before protect() was ever called. On that early return the partially-built value is dropped, and Drop for Handlers unconditionally calls unprotect():

• In debug/CI-assert builds, debug_assert!(self.protection_count > 0) fires and the process aborts.
• In release builds it calls JSValue::unprotect() on callbacks that were never protected, which can release a protection legitimately held elsewhere on the same function objects.

The Zig reference (Handlers.zig) never deinits the partially-built struct on these error paths; the Rust port's Drop introduced the imbalance.

This change validates the callbacks (and the data/drain requirement) before constructing the Handlers value, so a Handlers only ever exists in its protect()ed state and every drop stays balanced. Error messages and validation order are unchanged.

How did you verify your code works?

• Minimized fuzzer repro (Bun.connect({ socket: {} }), also with a circular tls object + Bun.gc(true)) panicked before the fix and now throws the expected Expected at least "data" or "drain" callback error / exits cleanly with a debug build.
• Added tests in test/js/bun/net/socket.test.ts covering missing and non-callable handlers for both Bun.connect and Bun.listen; they pass with bun bd test.
• Ran the full test/js/bun/net/socket.test.ts suite with the debug build: same pass/fail set as the unmodified baked Bun in this environment (remaining failures are pre-existing local ECONNREFUSED issues unrelated to this change).

[coderabbitai]

Warning

Review limit reached

@robobun, we couldn't start this review because you've used your available PR reviews for now.

Your plan currently allows 5 reviews/hour. Refill in 47 minutes and 43 seconds.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more review capacity refills, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4228e60a-fed2-49fd-9c6e-33eb0e44242c

📥 Commits

Reviewing files that changed from the base of the PR and between 49c97de and e5d1b59.

📒 Files selected for processing (2)

• src/runtime/socket/Handlers.rs
• test/js/bun/net/socket.test.ts

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[robobun]

^{Updated 5:26 PM PT - May 24th, 2026}

❌ @robobun, your commit e5d1b59 has 2 failures in Build #57659 (All Failures):

• test/cli/install/bun-install-security-provider.test.ts - code 1 on 🪟 11 aarch64
• Failed to create agent for 🪟 11 aarch64 - test-bun

---

🧪   To try this PR locally:

bunx bun-pr 31360

That installs a local version of the PR into your bun-31360 executable, so you can run:

bun-31360 --bun

[github-actions]

This PR may be a duplicate of:

1. socket: don't assert in Handlers::unprotect when never protected #30948 - Also fixes the protection_count > 0 assertion crash in Handlers::unprotect when socket handler validation fails, but uses a defensive check (skip unprotect when count is 0) rather than restructuring validation order

🤖 Generated with Claude Code

[claude]

LGTM — straightforward reordering so validation errors return before Handlers is constructed, keeping the Drop → unprotect() invariant intact.

Extended reasoning...

Overview

This PR touches one function, Handlers::from_generated in src/runtime/socket/Handlers.rs, plus adds four regression assertions in test/js/bun/net/socket.test.ts. The change moves callback validation (the is_callable() checks and the data/drain-required check) to before the Handlers struct is constructed, instead of after. The macro body, validation order, and error messages are byte-identical to before — only the sequencing changes so that early return Err(...) paths no longer drop a Handlers value whose Drop impl unconditionally calls unprotect() on never-protect()ed callbacks.

Security risks

None introduced. The change actually closes a release-build hazard where JSValue::unprotect() could be called on values that were never protected, potentially releasing a GC root held elsewhere. No new FFI surface, no user-controlled data handling changes, no auth/crypto involvement.

Level of scrutiny

Medium — this is GC-root lifecycle code in the Rust port of the socket layer, so correctness matters for memory safety. However, the change is a pure mechanical reordering within a single function: the success path is semantically identical (same fields assigned the same values, same with_async_context_if_needed + protect() tail), and the error paths now simply return before any Handlers exists. There are no new control-flow branches and no changes to Drop, protect, or unprotect themselves.

Other factors

• The bug hunting system found no issues.
• Regression tests cover all four error shapes (empty handlers and non-callable handlers, for both Bun.connect and Bun.listen).
• No CODEOWNERS entry covers these paths.
• No outstanding human review comments on the PR.
• The PR description clearly explains the root cause (Rust Drop semantics diverging from the Zig reference, which never deinits on these error paths) and the fix matches that analysis exactly.

[robobun]

Re the duplicate flag: #30948 was an earlier fix of mine for the same fuzzer fingerprint that skipped the unprotect when protection_count == 0 (debug builds only). I've closed it in favor of this PR — validating before constructing the Handlers keeps the assertion meaningful and also stops release builds from calling unprotect() on callbacks that were never protected.

[robobun]

CI note: build #57649 had no test failures — all 73 jobs that ran passed. The only red entries were darwin-14-aarch64-test-bun and darwin-26-aarch64-test-bun, both Expired (no agent picked them up), the same macOS agent issue seen on recent builds. Pushed an empty commit to re-run.

[claude]

LGTM — validating callbacks before constructing Handlers cleanly closes the drop-before-protect gap, and the success path is byte-for-byte equivalent.

Extended reasoning...

Overview

This PR reorders Handlers::from_generated in src/runtime/socket/Handlers.rs so that callback validation (callable check + data/drain presence check) happens before the Handlers struct is constructed, rather than after. Previously, an early return Err on invalid handlers would drop a partially-built Handlers whose Drop impl calls unprotect() — tripping debug_assert!(protection_count > 0) in debug builds and unbalancing JSC protect counts in release builds. A regression test is added to test/js/bun/net/socket.test.ts.

Security risks

None. This is an input-validation ordering fix on a user-facing error path; it does not touch auth, crypto, permissions, or network I/O. The release-build behavior it fixes (spurious JSValue::unprotect() on never-protected values) was a GC-safety hazard, so the change is strictly a hardening.

Level of scrutiny

Low-to-moderate. The diff is a mechanical hoist: the callback! macro is the old assign_callback! macro reshaped to return the value instead of assigning into result.$field, with identical branching and error messages. Field iteration order is unchanged, so user-visible validation order and messages are preserved. After construction there are no remaining fallible operations before protect() (with_async_context_if_needed is infallible FFI), so the stated invariant — a Handlers only ever exists protected — holds.

Other factors

• Regression tests cover both Bun.connect and Bun.listen, both error variants (missing data/drain, non-callable handler).
• No CODEOWNERS entry for these paths.
• The bug-hunting system found no issues.
• The author already closed the weaker alternative fix (#30948) in favor of this approach, and CI "failures" were macOS agent expirations, not test failures.

[robobun]

Final status for build #57659: the macOS lanes that had expired eventually ran and passed, so the only red lane is windows-11-aarch64-test-bun, which failed solely because test/cli/install/bun-install-security-provider.test.ts → "handles packages JSON larger than max arg length (>1MB)" hit its 120s timeout on that agent (retried several times within the job, timing out each time). That install/IPC test is untouched by this PR and unrelated to the socket-handlers change; test/js/bun/net/socket.test.ts ran in a different shard and passed, as did every other platform including the ASAN lane.

The fix itself is complete and verified. I've already used one CI retrigger for this PR, so I'm not pushing another — a retry of that single lane (or treating it as the known slow-agent timeout) is all that's left.

[robobun]

Superseded by #31817, which fixes the same crash (panic: assertion failed: self.protection_count > 0 from Handlers::Drop → unprotect() on validation-error paths in Handlers::from_generated) with the same validate-before-construct approach, authored by @alii.

Verified on a debug build of #31817 that both of this PR's repro shapes now throw cleanly instead of aborting:

• Bun.connect({ hostname, port, socket: {} }) → ERR_INVALID_ARG_TYPE: Expected at least "data" or "drain" callback
• non-callable handlers (data: 123, drain: 1) → ERR_INVALID_ARG_TYPE: Expected "…" callback to be a function

The regression test in #31817 now covers both validation error paths (non-callable handler + missing data/drain), so this PR's test coverage is subsumed. Closing in favor of #31817.