issue: 4923648 Fix segfault: Ultra API sockets coexist w/ worker threads

[tomerdbz]

Description

When worker_threads > 0, XLIO unconditionally dispatches any socket's connect/listen/accept operations to worker threads via distribute_socket(). Ultra API sockets manage their own poll groups and rings, so this dispatch causes double-ownership: the application thread drives the socket through its poll_group while the worker thread also tries to manage it. This leads to "Buff is already a member in a list!" errors and a SIGSEGV in TCP_EVENT_ERR when the worker thread invokes a NULL callback.

Add sockinfo::should_use_threads_mode() which returns true only when worker threads are configured AND the socket is not an Ultra API socket (m_is_xlio_socket == false). Replace the three bare is_threads_mode() / worker_threads > 0 checks in sockinfo_tcp.cpp (connect, listen, accept paths) with this new method, allowing POSIX and Ultra API sockets to coexist safely when worker threads are enabled.

Add a regression test -ultra_api_worker_threads.no_distribute_on_connect that exercises xlio_socket_connect() with worker_threads=1 at debug log level and asserts that worker-thread dispatch messages are absent from the output. Extract subprocess test infrastructure from output/config.cc into common/subprocess_test.h for reuse.

What

Skip worker-thread dispatch for Ultra API sockets, preventing SIGSEGV when Ultra API and worker threads coexist.

Why ?

When worker_threads > 0, XLIO unconditionally dispatches any socket's connect/listen/accept to worker threads via distribute_socket(). Ultra API sockets manage their own poll groups and rings, so this dispatch causes double-ownership: the application thread drives the socket through its poll_group while a worker thread also tries to manage it. This leads to "Buff is already a member in a list!" errors and a SIGSEGV in TCP_EVENT_ERR when the worker thread invokes a NULL callback. See Redmine #4923648.

How ?

Add sockinfo::should_use_threads_mode() which returns true only when worker threads are configured and the socket is not an Ultra API socket (!m_is_xlio_socket). Replace the three bare is_threads_mode() / worker_threads > 0 checks in sockinfo_tcp.cpp (connect, listen, accept paths) with this method. POSIX sockets continue to be dispatched to worker threads as before; Ultra API sockets bypass the dispatch entirely.

A regression test (ultra_api_worker_threads.no_distribute_on_connect) launches a helper binary that calls xlio_socket_connect() with worker_threads=1 at debug log level, then asserts that worker-thread dispatch messages (connect_socket_job) are absent from the output. The test runs automatically in CI under the ultra_api* filter with real RDMA addresses and root privileges. Subprocess test infrastructure is extracted from output/config.cc into common/subprocess_test.h for reuse.

Change type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• Tests
• Other

Check list

• Code follows the style de facto guidelines of this project
• Comments have been inserted in hard to understand places
• Documentation has been updated (if necessary)
• Test has been added (if possible)

Summary by CodeRabbit

• Bug Fixes

  • Improved connection and accept handling when worker-thread mode is enabled, making behavior more consistent across socket types.

• Tests

  • Added new end-to-end coverage for worker-thread connection scenarios.
  • Introduced shared test support for running helper programs and capturing their output.
  • Updated test configuration and build setup to include the new scenarios.

[greptile-apps]

Greptile Summary

This PR fixes a segfault that occurs when Ultra API sockets coexist with XLIO worker threads. The root cause was that distribute_socket() dispatched all sockets to worker threads unconditionally, creating double-ownership for Ultra API sockets that manage their own poll groups.

• Adds sockinfo::should_use_threads_mode() which returns true only for non-Ultra-API sockets when worker_threads > 0, and applies it to all four thread-mode guards in the connect, listen, and accept_helper code paths in sockinfo_tcp.cpp.
• Introduces a regression test (ultra_api_worker_threads.no_distribute_on_connect) that launches a helper binary with worker_threads=1 and asserts that worker-thread dispatch log messages are absent; extracts reusable subprocess test infrastructure into common/subprocess_test.h and refactors the output fixture to inherit from it.

Confidence Score: 5/5

The fix is minimal and surgical — one new predicate method, four call-site replacements — and all four thread-mode guards in the connect/listen/accept code paths have been updated consistently.

The new should_use_threads_mode() correctly composes the existing is_threads_mode() (confirmed equivalent to worker_threads > 0) with the !m_is_xlio_socket guard. Both the POSIX socket path (behavior unchanged) and the Ultra API socket path (dispatch skipped) are correctly handled. The regression test adds a meaningful negative assertion with a positive 'test was not vacuous' check. No issues found in the changed code paths.

No files require special attention. The core change in sockinfo_tcp.cpp is straightforward and fully covered by the new regression test.

Important Files Changed

Filename	Overview
src/core/sock/sockinfo.h	Adds should_use_threads_mode(): returns is_threads_mode() && !m_is_xlio_socket. Correctly gates Ultra API sockets out of worker-thread dispatch.
src/core/sock/sockinfo_tcp.cpp	Replaces all four raw worker_threads > 0 / is_threads_mode() checks (connect, listen, two in accept_helper) with should_use_threads_mode(). All call sites correctly updated.
tests/gtest/common/subprocess_test.h	New base class for subprocess-launching tests; includes explicit read_file open-failure check and pid-scoped temp file paths. Clean extraction.
tests/gtest/output/config.cc	Refactored to extend subprocess_test; removes duplicated exec_cmd_to_file / read_file / workspace logic. TearDown correctly inherited from base.
tests/gtest/xlio_ultra_api/xlio_socket_worker_threads.cc	Regression test for the fix: launches helper binary with worker_threads=1 and asserts absence of dispatch log messages. Positive marker assertion prevents vacuous passes.
tests/gtest/xlio_ultra_api/ultra_api_connect_worker_threads_helper.c	Helper binary that exercises xlio_socket_connect() under worker_threads=1. Handles SKIP gracefully for non-RDMA environments. Return codes correctly map to test outcomes.
tests/gtest/Makefile.am	Adds helper binary as a separate noinst_PROGRAMS target and sets GTEST_BUILDDIR via -DGTEST_BUILDDIR=$(abs_builddir), enabling out-of-tree build path resolution.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["sockinfo_tcp::connect / listen / accept_helper"]
    B{"should_use_threads_mode()\nis_threads_mode() && !m_is_xlio_socket"}
    C["Worker-thread dispatch path\n(distribute_socket / harvest_sockinfo_tcp_listen_objects\nassert m_syn_received.empty())"]
    D["Standard path\n(connect_threads_mode skipped / attach_as_uc_receiver\nremove_received_syn_socket)"]
    E["POSIX socket\nm_is_xlio_socket == false"]
    F["Ultra API socket\nm_is_xlio_socket == true"]

    A --> B
    E --> B
    F --> B
    B -->|"true (POSIX + worker_threads>0)"| C
    B -->|"false (Ultra API OR no worker threads)"| D

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["sockinfo_tcp::connect / listen / accept_helper"]
    B{"should_use_threads_mode()\nis_threads_mode() && !m_is_xlio_socket"}
    C["Worker-thread dispatch path\n(distribute_socket / harvest_sockinfo_tcp_listen_objects\nassert m_syn_received.empty())"]
    D["Standard path\n(connect_threads_mode skipped / attach_as_uc_receiver\nremove_received_syn_socket)"]
    E["POSIX socket\nm_is_xlio_socket == false"]
    F["Ultra API socket\nm_is_xlio_socket == true"]

    A --> B
    E --> B
    F --> B
    B -->|"true (POSIX + worker_threads>0)"| C
    B -->|"false (Ultra API OR no worker threads)"| D

Loading

_{Reviews (12): Last reviewed commit: "issue: 4923648 Fix segfault: Ultra API s..." | Re-trigger Greptile}

[ntsemah]

bot:retest

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Enterprise

Run ID: 858442ce-4aed-4540-85f1-ff95eeedf9f6

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[tomerdbz]

bot:retest

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

tests/gtest/common/subprocess_test.h (1)

7-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Header isn't self-contained.

subprocess_test.h uses testing::Test (Line 21) and unlink() (Line 36) without including <gtest/gtest.h> or <unistd.h>. It currently compiles only because both consumers happen to include these transitively first; any new consumer that includes this header without that ordering would fail to build.

♻️ Proposed fix

+#include <gtest/gtest.h>
+#include <unistd.h>
 `#include` <cstdlib>
 `#include` <fstream>
 `#include` <string>
 
 `#include` "common/def.h"

Also applies to: 21-38

🤖 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 `@tests/gtest/common/subprocess_test.h` around lines 7 - 15,
`subprocess_test.h` is not self-contained because `SubprocessTest` inherits from
`testing::Test` and uses `unlink()` without including their defining headers.
Update the header to include the proper declarations directly so it can stand on
its own, specifically by adding the missing gtest and POSIX header alongside the
existing includes. Keep the fix localized to the `SubprocessTest` definition and
its use of `unlink()`.

🤖 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 `@src/core/sock/sockinfo_tcp.cpp`:
- Around line 3344-3345: Keep the accept cleanup gated by the same threads-mode
predicate used in the listen/receive path. In
sockinfo_tcp::sockinfo_tcp::rx_wait/related accept handling, replace the raw
safe_mce_sys().worker_threads check with should_use_threads_mode() so Ultra API
sockets follow the same branch as harvest_sockinfo_tcp_listen_objects(). Make
sure remove_received_syn_socket(ns) still runs whenever the threads-mode path is
active, preventing stale non-thread accept state and related assertions.

In `@tests/gtest/xlio_ultra_api/xlio_socket_worker_threads.cc`:
- Around line 44-46: The test docstring for xlio_socket_worker_threads is out of
sync with the actual assertions: it claims both "connect_socket_job" and "New
TCP socket added" are checked, but only one marker is currently verified. Update
the test body in the socket worker thread regression test to explicitly assert
the absence of the "New TCP socket added" dispatch message as well, keeping the
comments and assertions aligned and covering the entity_context marker alongside
the existing vlist/crash checks.

---

Nitpick comments:
In `@tests/gtest/common/subprocess_test.h`:
- Around line 7-15: `subprocess_test.h` is not self-contained because
`SubprocessTest` inherits from `testing::Test` and uses `unlink()` without
including their defining headers. Update the header to include the proper
declarations directly so it can stand on its own, specifically by adding the
missing gtest and POSIX header alongside the existing includes. Keep the fix
localized to the `SubprocessTest` definition and its use of `unlink()`.

🪄 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: Repository UI

Review profile: CHILL

Plan: Enterprise

Run ID: 26b9b596-b3cf-4232-9066-f46570c20f26

📥 Commits

Reviewing files that changed from the base of the PR and between 35b6c7d and ff44d59.

📒 Files selected for processing (9)

• .gitignore
• src/core/sock/sockinfo.h
• src/core/sock/sockinfo_tcp.cpp
• tests/gtest/Makefile.am
• tests/gtest/common/subprocess_test.h
• tests/gtest/output/config.cc
• tests/gtest/xlio_ultra_api/config-ultra-api-worker-threads.json
• tests/gtest/xlio_ultra_api/ultra_api_connect_worker_threads_helper.c
• tests/gtest/xlio_ultra_api/xlio_socket_worker_threads.cc

[tomerdbz]

bot:retest

[tomerdbz]

The three open review threads here were filed against the pre-squash commit ff44d595. The branch was subsequently squashed and force-pushed to the current head 432a5213, which already contains all three fixes, so the threads did not auto-resolve. Evidence (git blame attributes each line to 432a5213):

1. sockinfo_tcp.cpp accept path now uses should_use_threads_mode() at both the harvest (:3344) and cleanup (:3372) sites - no raw worker_threads/is_threads_mode() check remains in that path.
2. tests/gtest/common/subprocess_test.h: SetUp() now initializes m_output_file (~L35), symmetric with the TearDown() that deletes it.
3. tests/gtest/xlio_ultra_api/xlio_socket_worker_threads.cc now asserts the documented dispatch marker (~L80-85), matching the docstring at L45.

@orshemesh-grok could you re-review 432a5213 and clear the change request if this looks good? Thanks.