Release handover connections directly instead of through disown()

[kwsantiago]

Fixes a pre-existing defect in the vendored epoll worker where a connection handed over in the same batch as a WebSocket upgrade could be dropped from every tracking list, leaking its slot and fd and pinning a CPU core at 100% indefinitely.

The bug

processSignal snapshots handover_list by taking inner.head and clearing inner, but the snapshot nodes keep their next/prev. disown() then dispatches on _state, which is still .handover, so it calls handover_list.remove() on a node that is no longer a member.

The general rule is that a snapshot node's links are stale with respect to the live list, and List.remove rewrites head/tail from exactly those two fields. Worker threads keep inserting into that live list throughout, so there is always something to corrupt. Two reachable shapes:

• [A(.close)] alone: prev and next are both null, so remove(A) sets head = null and tail = null, wiping an entry inserted after the snapshot was taken. No WebSocket involved.
• [B(.websocket), A(.close)]: the .websocket branch neither removes its node nor releases it, so A.prev is still B. remove(A) leaves head null and writes tail = B, a non-member; since List.insert only assigns head when tail == null, every later handover appends behind B until the next drain resets the list.

Either way the affected connections are in no list at all: the slot is never released, the fd is never closed, the HTTPConn is never returned, and they are in neither timeout list, so no sweep can ever reach them.

Worse, its epoll registration is the level-triggered IN|RDHUP from accept, so once the peer sends anything or half-closes, epoll_wait returns it every iteration and run() skips it via the .handover state check. That is an unbounded busy spin on the worker's event-loop thread.

This is the same defect class as the timeout-sweep fix that recently landed upstream, in the sibling code path.

Reachability

Remote and unauthenticated: alternate WebSocket upgrades with requests that end in a .close handover (Connection: close, an HTTP/1.0 GET / for NIP-11, or an upgrade rejected by the per-IP limiter, which answers 429 and then hands over as .close).

The fix

releaseHandover() releases a connection the snapshot already detached, without touching the list. disown() is still correct for its other callers, whose connections genuinely are members of the list they name.

Clearing next/prev at snapshot time would not work and is called out in a comment: remove() would then null head/tail outright, discarding entries that worker threads inserted after the snapshot was taken.

Verification

Reproduced first, then fixed. Measured under identical load (interleaved upgrades and close-handovers against a single worker), with a temporary probe compiled in to detect the corrupt head == null, tail != null state directly rather than inferring it:

	poisoning events	socket fds after settle
before	18	42
after	0	1

The unfixed relay also sat at 99.6% CPU for over six minutes after the load stopped, confirming the busy spin.

tests/integration_handover_leak.sh guards it in CI, asserting both symptoms: no CPU burn while idle, and no orphaned fds. Validated by mutation, since a test that cannot fail proves nothing:

• fixed: 6/6 pass, 0 CPU ticks over 5s idle, fds flat at baseline
• reverting only releaseHandover() back to disown(): fails with 500 ticks over 5s (exactly one full core) and orphaned fds over the threshold

An earlier, weaker version of this test passed on the unfixed build; it needed each upgrade held briefly so its handover sits adjacent to a close, which is the poisoning precondition.

Also green: zig build test 65/65, protocol suite 45/45, concurrency 2/2 at 200 connections, and scripts/verify-vendored-httpz.sh.

Review follow-ups

• The explanation above originally claimed the corruption required a .websocket node in the snapshot. It does not: a lone .close node wipes both head and tail. Corrected here and in the code comment, since the narrower claim would mislead the next reader into thinking only upgrade traffic can trigger it.
• The test could pass on a corpse: if the relay died during the settle, both the fd count and the CPU reading degrade to 0 and every assertion printed ok. It now fails closed on a dead pid.
• The load generators swallowed connect failures, so refused connections were indistinguishable from completed ones. The test now counts successful connects and asserts a floor (CI reports 480 upgrades and 480 close-handovers against a floor of 240).
• The .handover arm of disown() is now unreachable. Left as a plain remove rather than unreachable, so that a wrong analysis degrades to a stale list entry instead of aborting a live relay, with a comment saying so.
• The surviving .websocket node's links are nulled, so the "nothing dereferences them" invariant is local rather than resting on List.insert overwriting both fields.

[coderabbitai]

Walkthrough

The relay now uses separate cleanup paths for detached handovers and WebSocket teardown. The new Linux integration test creates concurrent handover traffic, checks resource cleanup, and runs in CI against a single-worker relay.

Changes

Handover cleanup

Layer / File(s)	Summary
Detached handover reclamation vendor/httpz.patch, vendor/httpz/src/worker.zig	Detached handovers release HTTP connections, wrappers, and connection slots without mutating the detached list. Timeout cleanup uses guarded slot release.
WebSocket teardown flow vendor/httpz.patch	WebSocket teardown queues connections for event-loop reclamation. Closed and invalid handovers use the detached cleanup path.
Leak regression validation tests/integration_handover_leak.sh, .github/workflows/ci.yml	The test drives concurrent handover traffic, checks CPU and socket cleanup, verifies metrics, and runs against a single-worker relay in CI.

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

Suggested reviewers: wksantiago

Poem

I’m a rabbit who watched the handovers flow,
Detached links now cleanly go.
WebSockets queue their final flight,
Leak checks guard the relay night.
CI hops along, and sockets stay light.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main fix: directly releasing handover connections instead of using disown().

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/handover-list-corruption

---

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.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

.github/workflows/ci.yml (1)

274-277: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Print relay-handover.log when the test fails.

The step only dumps the log if the relay never listens. If the leak assertions fail, no relay output is shown, so a CI failure gives the fd and tick counts without any relay context. Add a conditional dump before exit $rc.

♻️ Proposed change

           bash tests/integration_handover_leak.sh ws://127.0.0.1:7788 "$pid"
           rc=$?
+          [ "$rc" -eq 0 ] || cat relay-handover.log 2>/dev/null || true
           stop "$pid"; wait "$pid" 2>/dev/null
           exit $rc

🤖 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 @.github/workflows/ci.yml around lines 274 - 277, Add a conditional
failure-only dump of relay-handover.log after the cleanup commands in the
integration handover test step and before exit $rc. Preserve the existing
successful path without log output, and ensure the dump runs when the test
command returns a nonzero status.

🤖 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/integration_handover_leak.sh`:
- Line 74: Update the upgrade connection block at
tests/integration_handover_leak.sh:74-74 and the closereq connection block at
tests/integration_handover_leak.sh:85-85 to record connection failures or exit
non-zero instead of silently using exit 0; before evaluating the file-descriptor
and tick assertions, require at least the minimum number of successful
connections so the test confirms the generators exercised releaseHandover().

---

Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 274-277: Add a conditional failure-only dump of relay-handover.log
after the cleanup commands in the integration handover test step and before exit
$rc. Preserve the existing successful path without log output, and ensure the
dump runs when the test command returns a nonzero status.

🪄 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: 22bb510e-71d6-4e66-8099-a817dafc9570

📥 Commits

Reviewing files that changed from the base of the PR and between e0a799c and 1293b89.

📒 Files selected for processing (4)

• .github/workflows/ci.yml
• tests/integration_handover_leak.sh
• vendor/httpz.patch
• vendor/httpz/src/worker.zig