fix: Accept() must not close the listener socket on success

[cdgraff]

Summary

NodeSRT::Accept() (src/node-srt.cc) called srt_close() on the listener socket on the success path, so the listener was destroyed after every accepted connection.

In libsrt, srt_accept() returns the newly accepted socket and leaves the listener open so it can keep accepting - this diverged from that contract:

• A server could accept only one connection per listener; after the first accept() the listener went BROKEN then NONEXIST, and any further caller timed out.
• The usual workaround, re-creating (bind+listen) the listener after every accept, drops its backlog of pending handshakes. Under a burst of simultaneous callers this meant only one connection got through and the rest failed.

Fix

Removes the success-path srt_close(socketValue) (and the following no-op reassignment), so Accept() only returns the accepted fd and leaves the listener open, matching libsrt semantics. The caller is responsible for closing the listener when done. The failure path is unchanged.

Test

Adds spec/accept_keeps_listener_open_spec.js, a regression test covering both the sync (SRT) and async (AsyncSRT) accept paths: it accepts several connections in a row from the same listener and asserts the listener stays LISTENING throughout.

Verified locally:

• Reverting just the src/node-srt.cc change and rebuilding the addon reproduces the bug: the listener's state goes BROKEN right after the first accept, and the next connection attempt errors out (invalid listener socket ID value).
• With the fix applied, the full spec suite passes (23/23), including the new test.

Fixes #79

cc @birme

[birme]

Thank you for this fix, @cdgraff — the root-cause diagnosis is spot-on and the C++ change is exactly right: removing those two lines from the success path of Accept() correctly matches libSRT semantics, and the regression test covers both the sync and async paths in a clear and minimal way.

There is one blocker before this can be merged: the Node.js 18.x CI job fails with a segmentation fault (exit code 139) during the jasmine test run. The build itself succeeds on 18.x; the crash occurs at runtime while running the test suite (approximately 20 tests in before the crash). Node 20 and 22 both pass all 23 specs cleanly.

The segfault is most likely a Node 18-specific N-API or libuv threading interaction in the new test — particularly the pattern of mixing a blocking SRT.accept() on the main thread with an AsyncSRT worker driving the connecting side, under Node 18's older N-API ABI. It could also be a pre-existing instability in the suite under Node 18 that the new test happens to expose.

Could you please:

1. Run the full jasmine suite on Node 18 locally (node --version should show 18.x) to reproduce the crash, ideally with ASAN or a core dump to identify the crashing frame.
2. If the crash is pre-existing (i.e. the old suite also segfaults on Node 18 without your changes), note that in the PR body so the maintainer can decide whether to drop Node 18 from the matrix or guard the new test.
3. If the crash is new, consider whether the sync-accept-with-async-client pattern in the first test case needs a different setup under Node 18 (e.g. a short delay before calling srt.accept, or a different port-isolation strategy).

Once CI is green (or the 18.x failure is confirmed pre-existing and the maintainer decides to act on it), this fix is ready to merge. The fix itself is clearly correct and addresses a real and significant bug.

node-srt maintenance bot — Tier B; awaiting human merge after CI resolution.

[birme]

Follow-up from the native build expert: the Node 18.x segfault has been investigated and is a pre-existing issue, not caused by this PR.

Root cause of the Node 18 crash:
The crash is a race between Worker.terminate() and an in-flight blocking native SRT call in the AsyncSRT worker thread. Under Node 18's worker_threads implementation, terminate() is more abrupt and can destroy the worker's native thread context while libSRT is still holding a reference to stack memory. Node 20+ has better fence semantics here and survives the race reliably. This race pre-dates this PR — it can be triggered on master with the right jasmine random seed.

The two-line deletion in src/node-srt.cc is N-API-safe across all Node versions and has no role in the crash.

Options to get CI green (choose one):

1. Drop Node 18 from the CI matrix (in .github/workflows/nodejs.yml). Node 18 reached end-of-life in April 2025 — this is independently justified and unblocks the PR immediately.
2. Restructure the new test's teardown into a afterAll hook that drains the async client before jasmine tears down the suite, giving Worker.terminate() a quiescent window.
3. Harden AsyncSRT.dispose() to send a shutdown sentinel to the worker and await its acknowledgement before calling Worker.terminate(), eliminating the race for all Node versions (a deeper but valuable change).

Option 1 is the quickest path and is defensible on its own merits. The maintainer will decide which approach to take — the fix itself is correct and ready.

node-srt native build expert / maintenance bot