net: format socket errors with a thread-safe renderer (fixes #625)

[Yaraslaut]

Fixes #625.

TcpSocket formatted every socket error with std::strerror, which is
permitted to return a pointer to one static buffer shared by all callers, and
every one of those call sites runs on whichever thread hit the error. The
subsystem spawns those threads itself, so two of them can be inside a throw
site at once.

The change

• A private TcpSocket::errnoMessage(int) over
  std::system_category().message(), used at all seven std::strerror sites.
  It returns an owned std::string and carries the library's ordinary "shall
  not introduce a data race" guarantee. Chosen over strerror_r, whose XSI and
  GNU variants differ in return type and so need a build-time discriminator and
  a caller-supplied buffer.
• The hand-written NOLINTNEXTLINE(concurrency-mt-unsafe) at the tryAccept()
  site is removed with its subject, per Four NOLINTNEXTLINE directives are wrapped onto two lines and suppress nothing; six findings leak past them #627's lesson.
• docs/spec/security.md's morph::net section states the property, including
  what was and was not measured.
• One test pinning the message shape and the category that renders it.

The one claim the branch's safety rests on

That std::error_category::message may be called concurrently, where
std::strerror may not. It is a specification claim, not a measurement:
std::strerror is one of the functions the C and C++ standards permit to use a
shared static buffer, and error_category::message is subject to
[res.on.data.races] with no carve-out. If that reading is wrong, the change is
merely neutral rather than harmful — the message text is unchanged (measured
below) — but the issue would not be fixed.

Reachability — the ticket's open question, answered

#625 asked whether all the sites are reachable from more than one thread. They
are, and by construction rather than by accident:

• SocketServer::acceptLoop() runs on its own thread and calls tryAccept()
  (socket_server.hpp:95,263).
• SocketServer spawns one clientLoop thread per accepted connection
  (socket_server.hpp:280), each driving recvSome() (:376) and sendAll()
  (:414). N concurrent connections means N threads in those two sites.
• SocketBackend runs an I/O thread and a handler thread
  (socket_backend.hpp:106-107), calling recvSome() (:894) and sendAll()
  (:642).
• listen() and connect() are the weaker cases — they run on whatever thread
  constructs the server or backend — but they share the buffer with everything
  above, so a concurrent recvSome failure is enough.

So the ticket is not closable as invalid on its stated condition.

A correction to the ticket

The triage counted "eight call sites"; the true count is seven.
grep -c "std::strerror" returns 8 because line 331 is the NOLINT comment,
which contains the word:

include/morph/net/detail/tcp_socket.hpp:331:            // NOLINTNEXTLINE(concurrency-mt-unsafe) — std::strerror, as at every other throw site here
include/morph/net/detail/tcp_socket.hpp:332:            throw std::runtime_error(std::string{"TcpSocket::tryAccept: "} + std::strerror(err));

Three in listen(), one each in accept(), tryAccept(), recvSome(),
sendAll(). Nothing about the fix changes; the number in the issue does.

::gai_strerror: explicitly out of scope

TcpSocket::connect renders resolver failures with ::gai_strerror. Left
alone, deliberately: rc is an EAI_* code, not an errno value, so
std::system_category().message(rc) would render a confidently wrong string.
There is no drop-in substitution, and clang-tidy does not classify the function
as unsafe (measured: the same probe file flags std::strerror and not
::gai_strerror). Its thread-safety is nonetheless unestablished by this
project, so it is filed as #640 rather than swept in or left unrecorded.

Verification

Measured. clang-tidy 22.1.8 (the version CI pins), same invocation each
time, against a -DMORPH_BUILD_NET=ON -DCMAKE_BUILD_TYPE=Debug clang compile
database:

$ clang-tidy -p build/net --checks='-*,concurrency-mt-unsafe' \
    --header-filter='include/morph/.*' tests/net/test_tcp_socket.cpp

tree	findings	exit
a8511aa6 as-is	6	1
a8511aa6, NOLINT line deleted	7	1
this branch	0	0

The 7-finding run, verbatim, is the mutation that shows the check is not
vacuous — the check fails when the fix is absent:

include/morph/net/detail/tcp_socket.hpp:195:92: error: function is not thread safe [concurrency-mt-unsafe,-warnings-as-errors]
include/morph/net/detail/tcp_socket.hpp:206:90: error: function is not thread safe [concurrency-mt-unsafe,-warnings-as-errors]
include/morph/net/detail/tcp_socket.hpp:211:92: error: function is not thread safe [concurrency-mt-unsafe,-warnings-as-errors]
include/morph/net/detail/tcp_socket.hpp:276:75: error: function is not thread safe [concurrency-mt-unsafe,-warnings-as-errors]
include/morph/net/detail/tcp_socket.hpp:331:78: error: function is not thread safe [concurrency-mt-unsafe,-warnings-as-errors]
include/morph/net/detail/tcp_socket.hpp:352:81: error: function is not thread safe [concurrency-mt-unsafe,-warnings-as-errors]
include/morph/net/detail/tcp_socket.hpp:370:80: error: function is not thread safe [concurrency-mt-unsafe,-warnings-as-errors]
7 warnings treated as errors

Measured — the CI gate itself, reproduced locally. clang-tidy-diff.py
with CI's own arguments over git diff -U0 origin/master:

$ python3 /usr/share/clang/clang-tidy-diff.py -path build/net -p1 -j 4 \
    -extra-arg=-std=c++23 -extra-arg=-Wno-missing-include-dirs -quiet
Running clang-tidy in 2 threads...
exit=0

Not vacuous either: with errnoMessage's body mutated back to
std::string{std::strerror(err)}, the same harness exits 1 and names the line:

include/morph/net/detail/tcp_socket.hpp:470:67: error: function is not thread safe [concurrency-mt-unsafe,-warnings-as-errors]
  470 |     static std::string errnoMessage(int err) { return std::string{std::strerror(err)}; }

Measured — build and tests. -DMORPH_BUILD_NET=ON -DMORPH_BUILD_TESTS=ON,
clang 22.1.8, -Weverything + strict on: builds clean,
All tests passed (1112 assertions in 191 test cases). g++ 16.2.1 -std=c++23 -Wall -Wextra -fsyntax-only on the header: clean. clang-format
clean on both changed C++ files. Doxygen (MORPH_BUILD_DOCUMENTATION=ON,
WARN_AS_ERROR = FAIL_ON_WARNINGS): clean. check_spec_sync.sh,
check_spec_citations.sh, check_catch_test_names.sh,
check_nolint_directives.sh: all OK.

Measured — the new test is not evidence for this change, and says so.
It was run against the pre-change header and passed there too:

"TcpSocket::listen: bind() failed: Address already in use" equals:
"TcpSocket::listen: bind() failed: Address already in use"
All tests passed (1 assertion in 1 test case)

It is a standing guard on the message shape. It does fail when the shape
changes — mutating the prefix to bind failed: fails it — so it is not a
no-op, just not a detector of this defect.

Not measured, and not claimed. The race itself. No interleaved or
corrupted message was ever observed. The defect is what the specification of
std::strerror permits, inferred from the code plus the thread inventory
above. On glibc/Linux the two spellings render an errno to identical bytes
(measured, immediately above), so no before/after behaviour difference is
observable on the only platform CI runs.

Review notes

Reasoning done inline rather than via /code-review, per the lane's bounds.
Three things I looked for and did not find a problem with:

• errno clobbering. errnoMessage is called in the same expression that
  reads errno at three sites (listen's socket(), recvSome, sendAll).
  The read happens before the call in every case — the argument is evaluated
  first — and the other four sites already cached errno into err before
  any intervening ::close(). Unchanged from before.
• Include hygiene. <cstring> was in this header only for
  std::strerror; removed. std::size_t was relying on it transitively, so
  <cstddef> replaces it, and <system_error> is added. GCC and clang both
  compile the header standalone.
• Allocation on the error path. system_category().message() allocates,
  where strerror did not — but the surrounding code already builds a
  std::string and throws a std::runtime_error, so the throw path allocated
  either way.

Files

Touches only include/morph/net/detail/tcp_socket.hpp,
tests/net/test_tcp_socket.cpp and docs/spec/security.md, none of which is
held by #630/#635/#638/#639/#561. docs/spec/core/backend.md would have been
the other home for the spec note (check_spec_sync.sh accepts either for
net), but it is held by #639, so the note went to security.md, where the
morph::net transport's posture already lives.
scripts/branch_partial_allowlist.json cites socket_server.hpp and
socket_backend.hpp lines, not this header — no line it cites moves.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[Yaraslaut]

Runner verification (landing sweep, 2026-09-20)

The branch does what it says. Checked against the tree, not the report:

• No std::strerror call survives. The only two occurrences left in tcp_socket.hpp are inside the new helper's doc comment, explaining why it is gone.
• errnoMessage( appears 8 times — seven call sites plus the definition, which is exactly the lane's corrected count.
• The NOLINT is gone: grep -n "concurrency-mt-unsafe" over the branch's header returns nothing.
• The helper is static std::string errnoMessage(int err) { return std::system_category().message(err); }, with the thread inventory and the strerror_r rejection written into the comment above it.

The ticket's count was mine and it was wrong. My triage on #625 said "eight call sites"; it is seven. grep -c "std::strerror" returned 8 because line 331 was the NOLINT comment, which contains the word. The lane caught it and said so rather than quietly matching my number — the right call.

On the load-bearing claim. It is a specification claim (std::error_category::message carries the ordinary no-data-race guarantee; std::strerror is permitted a shared static buffer), and the lane labelled it as such rather than dressing it as a measurement. I am not in a position to measure it either, and I am accepting it on the same basis: the standard gives strerror an explicit carve-out and gives error_category::message none. If that reading is wrong the change is neutral, not harmful, which bounds the downside.

What I particularly want on the record, because it is the opposite of the usual failure here: the lane ran its own new test against the pre-change header, found it passed there too, and reported that the test is a standing guard rather than evidence for the fix. A test that passes in both worlds is exactly what AGENTS.md means by a control that measures nothing — the lane found that in its own work and published it instead of letting the green tick speak. Same for the near-miss it reported: it nearly filed an issue claiming HeaderFilterRegex matched nothing, traced it to its own grep for warning: when WarningsAsErrors: "*" makes the output say error:, and filed nothing.

Not merged this sweep: 7 pass, 23 pending. Left for the next sweep; pending PRs are not polled.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
include/morph/net/detail/tcp_socket.hpp	75.00%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!