Never abandon a partially-written buffer in UnixSocketHandler::write

[marchaase]

Never abandon a partially-written buffer in UnixSocketHandler::write

Draft — impact framing pending measurement. An earlier revision of this
description claimed this defect corrupts the terminal stream (chopped ANSI
escape sequences, stuck mouse reporting). That claim was wrong and has been
withdrawn — see Withdrawn claim
below. The socket-layer defect is real and proven; what it costs is being
measured now.

The bug

UnixSocketHandler::write is a partial-write loop — bytesWritten accumulates
across iterations — but its five-second EAGAIN give-up never consults it:

while (bytesWritten < int(count)) {
  w = ::send(fd, ((const char*)buf) + bytesWritten, count - bytesWritten, MSG_NOSIGNAL);
  if (w < 0) {
    if (localErrno == EAGAIN || localErrno == EWOULDBLOCK) {
      std::this_thread::sleep_for(std::chrono::milliseconds(1));
      if (time(NULL) > startTime + 5) {
        // Give up
        return -1;
      }
    }
    ...

So a write that has already put a prefix of the buffer on the wire can be
abandoned. The peer keeps that prefix — it cannot be recalled — while the caller
receives -1, which is indistinguishable from "nothing was sent". The caller
cannot tell how much of the buffer the peer actually holds.

send returns a short count whenever the socket has room for some but not all
of the buffer, so this is an ordinary state under backpressure, not a rare race.

Reproduction

Standalone, no ET code involved — it reimplements the loop above against a real
socket. Saturate a loopback TCP connection until send raises EAGAIN, have the
peer drain a small amount so a partial write becomes possible, then write a
payload much larger than the send buffer.

	Linux 6.16.1	macOS 25.6.0
SO_SNDBUF requested / actual	2048 / 4608	2048 / 65328
filler accepted before EAGAIN	8192	—
peer drained once	4096	4096
return code	-1	-1
bytes committed	31250 / 320000	11572 / 320000
0 < committed < count	True	True
elapsed	5.98 / 5.42 / 5.24s	—

Linux figures are three consecutive trials, identical every time.

The load-bearing result is 0 < committed < count together with rc == -1:
the write both sent data and reported total failure. That is the whole defect,
and it does not depend on what the bytes happened to be.

A note for anyone reproducing this: do not trust SO_SNDBUF. Requesting
2048 yields 4608 on Linux and 65328 on Darwin. Payloads smaller than the real
buffer fit entirely and reproduce nothing.

Withdrawn claim: no stream corruption

An earlier revision of this description argued the truncation corrupts the
terminal byte stream. It does not, and ET's design says so explicitly.

BackedWriter::write (src/base/BackedWriter.cpp:76) anticipates exactly this
ambiguity:

} else {
  // Error, we do not know how many bytes were written but it
  // does not matter because the reader is going to have to
  // reconnect anyways.  The important thing is for the caller to
  // think that the bytes were written and not call again.
  return BackedWriterWriteState::WROTE_WITH_FAILURE;
}

On -1 it does not assume nothing was written. It returns
WROTE_WITH_FAILURE; Connection.cpp:216 turns that into
closeSocketAndMaybeReconnect(); and recover(lastValidSequenceNumber) replays
every unacknowledged message from backupBuffer keyed by sequence number. The
truncated bytes die with the socket. Stream integrity is preserved at the
protocol layer.

This was confirmed end to end: a real etserver/et session over a
controllable tunnel, 60000 numbered markers each followed by ESC[?1003l, with
the tunnel stalled 8s — well past the 5s deadline — on both arms:

WITH fix    : 60000 markers, range 1..60000, 0 gaps, 0 duplicates, 0 chopped CSI
WITHOUT fix : 60000 markers, range 1..60000, 0 gaps, 0 duplicates, 0 chopped CSI

Identical. No corruption either way.

What it actually costs

Tracing the failure path rather than inferring it:

• Connection.cpp:202 is the only caller of writer->write().
• WROTE_WITH_FAILURE reaches closeSocketAndMaybeReconnect() on both
  branches at Connection.cpp:216.
• The give-up path leaves errno as EAGAIN from the last send, and
  isSkippableError (Connection.cpp:23) accepts EAGAIN — so it takes the
  "connection is severed" branch and tears the connection down at VLOG(1).

So the present behaviour under ordinary backpressure is a silent reconnect.
Correct, but not free: a reconnect is exactly the kind of hiccup a user notices.

Reconnect counts with and without the fix are being measured now, and this
section will be updated with them. If the fix does not reduce them, then it is
hygiene — a genuinely ambiguous return value made unambiguous — and this
description will say so plainly rather than claim an effect it cannot show.

The fix

The five-second budget is only safe while nothing has been committed; at that
point -1 is unambiguous and giving up is cheap. Once a byte is on the wire the
budget becomes SOCKET_WRITE_COMMITTED_TIMEOUT (60s), so the write is allowed
to finish rather than force a teardown. A real error from send still returns
immediately through the existing else branch.

If the committed budget does expire, the write still returns -1 — but it now
logs at ERROR with the fd, the committed count and the total, so the ambiguous
case is never silent.

Sixty seconds is a judgement call rather than a measured value.

A randomised soak over varying payload sizes and drain rates puts truncations at
29.2% before the change and 5.7% after — a large reduction, not zero. A
socket draining more slowly than the ceiling still truncates, and no choice of
ceiling removes that: the loop cannot un-send a prefix, and blocking forever is
worse.

Tests

Three cases added to test/unit_tests/UnixSocketHandlerTest.cpp (the existing
cases are untouched):

• WriteCompletesOnSlowlyDrainingSocket — a peer that drains slowly enough
  to hold the write blocked past five seconds. Asserts the write returns count
  rather than -1, and separately asserts it took longer than five seconds, so
  a too-fast drain cannot make the test green without exercising the regression.
  This case fails on the unpatched code.
• WriteStillGivesUpWhenNothingWasSent — the opposite guarantee. A full
  socket with a peer that never reads still gives up at ~5s, and asserts elapsed
  is under 30s so the committed budget cannot silently apply to everything.
• WriteFailsFastOnClosedPeer — a real error is not backpressure; must
  return in under two seconds without spinning out either budget.

Two take ~5-7s by construction — holding the socket blocked across the old
deadline is the whole point — and are tagged [slow].

They sit inside the file's existing #ifndef WIN32 block: they need
socketpair(AF_UNIX, …) and fcntl(O_NONBLOCK), so they are POSIX-only. The
production change itself is platform-independent and applies to the Windows
::send path too.

Test run

On macOS 25.6.0 / arm64, AppleClang 21, Catch2 v3.15.2.

Full suite with the fix applied:

100% tests passed out of 158

The [UnixSocketHandler] cases, showing the two [slow] ones spending their
time as intended rather than short-circuiting:

0.000 s: WriteFailsFastOnClosedPeer
5.792 s: WriteStillGivesUpWhenNothingWasSent
6.542 s: WriteCompletesOnSlowlyDrainingSocket
All tests passed (37 assertions in 8 test cases)

Reverting only src/base/UnixSocketHandler.cpp to master and rebuilding,
with the new tests left in place — the regression reproduces:

UnixSocketHandlerTest.cpp:208: FAILED:
  REQUIRE( rc == ssize_t(payload.size()) )
with expansion:
  -1 == 524288 (0x80000)

test cases:  3 |  2 passed | 1 failed

524288 bytes were asked for, a prefix went out, and the caller was handed -1.
The other two cases pass unpatched, which is the point of including them — they
pin down the behaviour the fix must not change.

bash format.sh (clang-format 18, Google style) produces no diff.

Relationship to #792

#792 (SO_LINGER) removed the process-wide five-second stalls that drove
sockets into backpressure, which made this defect far rarer without removing it.
The two are independent; this one was noted as a follow-up at the time.

[codecov]

Codecov Report

❌ Patch coverage is 92.85714% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.26%. Comparing base (b74a12e) to head (f6598bb).

Files with missing lines	Patch %	Lines
src/base/UnixSocketHandler.cpp	37.50%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #796      +/-   ##
==========================================
+ Coverage   87.14%   87.26%   +0.12%     
==========================================
  Files          75       75              
  Lines        6462     6532      +70     
  Branches      610      615       +5     
==========================================
+ Hits         5631     5700      +69     
  Misses        831      831              
- Partials        0        1       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.