Run the accept teardown once, and guard the shutdown union read

[kwsantiago]

Two remaining teardown defects in the vendored epoll worker. Both pre-existing and identical upstream.

1. accept() runs its whole teardown twice on a monitorRead failure

Three errdefers are live when loop.monitorRead(conn) fails, and they unwind LIFO, so all three run for one connection:

errdefer-C  conn.close() + disown()      <- closes the socket, destroys the Conn
errdefer-B  conn_mem_pool.destroy(conn)  <- destroys it a SECOND time
errdefer-A  posix.close(socket)          <- closes the fd a SECOND time

disown() already does the full teardown (list removal, slot release, http_conn_pool.release, conn_mem_pool.destroy), so the two earlier errdefers are pure duplication once the conn owns itself.

std.heap.MemoryPool.destroy prepends to a free list, so a double destroy links a node to itself and two later create() calls hand the same Conn to two different connections. The double close() can land on an fd another thread just opened.

Reproduced by fault injection, since the trigger is epoll_ctl failing (ENOMEM/ENOSPC against fs.epoll.max_user_watches), which is not directly attacker-controlled. Injecting a monitorRead failure every 5th accept and logging each errdefer gave exactly the predicted sequence, and the relay aborted with a core dump:

WISPPROBE injecting monitorRead failure
WISPPROBE errdefer-C conn.close + disown
WISPPROBE errdefer-B conn_mem_pool.destroy
WISPPROBE errdefer-A posix.close(socket)

The fix is a conn_owns_teardown flag set once the conn owns both its socket and its memory; the two earlier errdefers become no-ops from that point. Failures before that point still get the original cleanup, which is what they are there for.

Same injection against the fix, 8 failures: errdefer-A 0, errdefer-B 0, errdefer-C 8, no panics, relay alive.

2. shutdownList reads c.protocol.http with no union guard

This one turned out to be already unreachable, and I am recording why rather than just adding the guard.

The reported precondition was a processSignal snapshot leaving a stale .websocket node as handover_list's head. That required List.remove on a snapshot node, and after the handover-list and mutex fixes there is no handover_list.remove anywhere in the file. The list now has exactly one mutation site, the insert in swapList, always with a .http conn, plus the snapshot clear. Verified statically, and empirically across 12 shutdown-under-load rounds with upgrades in flight: zero sightings, zero panics, 12 clean shutdowns.

Guarded anyway, because that safety is a non-local invariant and the failure mode is severe: in ReleaseSafe the union read panics, and in ReleaseFast it reinterprets a *ws.HandlerConn as an *HTTPConn, so posix.close() closes whatever integer lands at that offset. server.deinit() runs before storage teardown, so a wild close there can land on the LMDB fd before the final sync. Skipping leaks one fd in a process that is already exiting, which beats either outcome.

Verification

zig build test 65/65, handover-leak 6/6, protocol 45/45, concurrency 2/2 at 200 connections, verify-vendored-httpz.sh clean, zero panics in any relay log.

Both fault-injection rigs were removed before the final build; the committed tree contains no probes.

Review follow-up

Both reviews came back with no blockers. Two things worth recording.

Applied: self.len += 1 sat seven lines above the ownership flag, and the two pre-flag errdefers release neither the slot nor the list entry. Any try added in that gap would have leaked a worker slot permanently, and past the insert would have left a freed Conn in the live request_list. The increment now sits immediately after request_list.insert(conn) and directly above the flag, with nothing fallible between them and a comment saying they must stay adjacent. Both reviews arrived at this independently. Re-verified afterwards, including the idle-reclaim test that reads slot reclamation back off the metrics gauge: 6/6.

Deliberately not applied here: the same unguarded c.protocol.http read remains in collectTimedOut, closeList and disown. Both reviews verified statically that none of them can currently see a .websocket conn, so this is consistency rather than a live bug, and the right remedy differs from the one used here. shutdownList runs during process teardown, so log-and-skip costs one leaked fd in an exiting process; those three run on every timeout sweep in a live relay, where skipping would leak a slot and an fd for the process lifetime, which is the accept-stall failure this project already fixed once. Picking the right degradation there deserves its own reproduction rather than riding along on this PR. Tracked separately.

Also confirmed by review, and worth stating because it inverts the usual intuition: the vendored posix.close is .BADF => unreachable, so the double close aborts in ReleaseSafe but is silent UB in ReleaseFast, which is what the release artifacts are built with. The loudly-crashing build was the safer one.

Summary by CodeRabbit

• New Features

  • Added public connection-count metrics for monitoring active accepted connections.

• Bug Fixes

  • Improved connection cleanup during failed accepts and shutdown.
  • Prevented duplicate socket and connection-resource destruction.
  • Improved WebSocket cleanup and handling of unexpected connection types.
  • Stabilized HTTP connection-pool recycling during concurrent activity.
  • Improved worker lifecycle handling to reduce connection leaks and shutdown issues.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: dad89d9e-76dc-4b85-adcf-7f97ddacb8ac

📥 Commits

Reviewing files that changed from the base of the PR and between 4b9075f and 8b17e90.

📒 Files selected for processing (2)

• vendor/httpz.patch
• vendor/httpz/src/worker.zig

---

Walkthrough

The patch adds public connection-count APIs and hardens HTTP and WebSocket connection ownership, handover cleanup, event-loop reclamation, shutdown protocol handling, slot accounting, and connection-pool recycling.

Changes

Connection lifecycle

Layer / File(s)	Summary
Accept ownership and slot accounting vendor/httpz.patch, vendor/httpz/src/worker.zig	Accept initialization transfers teardown ownership after request-list insertion and slot accounting. The patch exposes connectionCount() APIs.
Handover and WebSocket reclamation vendor/httpz.patch	Handover snapshots use dedicated release logic. Closed or failed WebSocket processing queues wrappers for event-loop cleanup.
Shutdown and pool release vendor/httpz.patch, vendor/httpz/src/worker.zig	Shutdown validates protocol variants before HTTP cleanup. Slot release is guarded, and HTTP pool recycling waits for mutex safety.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant WebSocketProcessing
  participant closeWebsocket
  participant EventLoop
  WebSocketProcessing->>closeWebsocket: Queue closed WebSocket wrapper
  closeWebsocket->>EventLoop: Signal the loop
  EventLoop->>EventLoop: Reclaim wrapper on loop thread

Loading

Possibly related PRs

• privkeyio/wisp#180: Modifies the same vendored lifecycle and WebSocket cleanup paths.
• privkeyio/wisp#181: Extends related handover cleanup changes in worker.zig.
• privkeyio/wisp#183: Includes related handover mutex and list-publication fixes.

Poem

I twitch my nose as sockets close,
Clean handovers keep the flow.
WebSocket wrappers leave the way,
The loop reclaims them without delay.
Slots count true, pools rest well—
Hop, hop, no leaks to tell!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes both primary fixes: single-run accept teardown and guarded shutdown union access.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/accept-teardown-double-free

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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