Half-close TCP connections so the last write is not lost#958
Conversation
close() went straight to shutdown(SHUT_RDWR) + close(). Doing that while either side still has unread data makes the stack send RST rather than FIN, and the device's FromRadio stream means there is essentially always unread data. Winsock discards data that was received but not yet read when an RST arrives, so an admin message written moments earlier is thrown away before the device reads it. Linux delivers the buffered bytes before reporting ECONNRESET, which is why this only surfaced on Windows: every one-shot TCP write there (--set, --seturl, --sendtext) reported success and did nothing. writeConfig() does not wait for an ack, so close() runs microseconds after sendall(). The device cannot compensate; it polls every 5ms and the RST beats its next read. Send FIN first with shutdown(SHUT_WR), which lets the device consume what we wrote, wait briefly for the reader thread to drain and exit, then tear down exactly as before. The full shutdown is kept so a reader still blocked in recv() is unblocked for the join in StreamInterface.close(). The wait is bounded: the device is not obliged to close just because we half-closed. Fixes #957
📝 WalkthroughWalkthrough
ChangesTCP graceful shutdown
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TCPInterface
participant Socket
participant Peer
participant ReaderThread
TCPInterface->>Socket: shutdown(SHUT_WR)
Socket-->>Peer: FIN permits remaining data consumption
TCPInterface->>ReaderThread: wait up to GRACEFUL_CLOSE_TIMEOUT
TCPInterface->>Socket: forced shutdown
TCPInterface->>Socket: close
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
meshtastic/tcp_interface.py (1)
117-119: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider skipping the wait if the half-close fails.
If
shutdown(SHUT_WR)raises an exception (e.g., the peer has already vanished or the socket is disconnected locally), the connection is already broken. In this case, waiting for the peer to consume data is unnecessary, and you can safely skip the 0.25-second delay by grouping the wait inside atryblock.💡 Proposed optional refactor
- with contextlib.suppress(Exception): - self.socket.shutdown(socket.SHUT_WR) - self._wait_for_reader_exit(GRACEFUL_CLOSE_TIMEOUT) + try: + self.socket.shutdown(socket.SHUT_WR) + self._wait_for_reader_exit(GRACEFUL_CLOSE_TIMEOUT) + except Exception: + pass🤖 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 `@meshtastic/tcp_interface.py` around lines 117 - 119, Update the socket close logic around self.socket.shutdown(socket.SHUT_WR) so _wait_for_reader_exit(GRACEFUL_CLOSE_TIMEOUT) runs only when the half-close succeeds. Keep exception suppression for shutdown failures and skip the wait when shutdown raises.
🤖 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.
Nitpick comments:
In `@meshtastic/tcp_interface.py`:
- Around line 117-119: Update the socket close logic around
self.socket.shutdown(socket.SHUT_WR) so
_wait_for_reader_exit(GRACEFUL_CLOSE_TIMEOUT) runs only when the half-close
succeeds. Keep exception suppression for shutdown failures and skip the wait
when shutdown raises.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 86b484f8-7bc9-4b4a-a3c3-2bc9a62bab34
📒 Files selected for processing (2)
meshtastic/tcp_interface.pymeshtastic/tests/test_tcp_interface.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #958 +/- ##
==========================================
+ Coverage 67.38% 67.40% +0.02%
==========================================
Files 25 25
Lines 4752 4762 +10
==========================================
+ Hits 3202 3210 +8
- Misses 1550 1552 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Looks reasonable to me. I'll pull it in shortly, just seeing if the simradio_testing daily job failure is actually anything or just noise. |
|
It happened twice in a row now. |
|
What actually fails. A single test: test_smokemesh_hop_limit_prevents_relay. It spins up simulated mesh nodes, has A send a broadcast with hopLimit=0, and asserts B receives it directly. B never receives it (assert col_b.wait_for(1) → False). This is firmware-side RF-sim packet delivery — it exercises none of the code your PR touches (the TCP client's graceful half-close). Your change can't cause it and can't fix it. Why "repeatedly." The daily PPA rebuilds nightly from firmware develop. PR #946's daily leg passed on 2026-07-07; PR #958's fails on 2026-07-17 — a different, later daily firmware. So a firmware change in that 10-day window altered hopLimit=0 direct-delivery behaviour (B no longer receives the direct broadcast), and every re-run of your PR pulls that same broken daily build, so it fails the same test every time. The stable alpha/beta firmware still behaves correctly. Bottom line: nothing to fix in PR #958 — the daily leg is informational and green-lights the merge regardless. The real signal is a firmware-side regression around hopLimit=0 delivery that belongs in the firmware repo, not here. |
2 participants
Fixes #957.
One-shot config writes over TCP are silently lost on Windows:
--setreportssuccess and the device never applies it. Reads are unaffected.
Cause
close()went straight toshutdown(SHUT_RDWR)+close(). Doing that whileeither side still has unread data makes the stack send RST rather than FIN,
and the device's FromRadio stream means there is essentially always unread data.
Winsock discards data that was received but not yet read when an RST arrives,
so the admin message written moments earlier is thrown away before the device gets
to read it. Linux delivers the buffered bytes before reporting
ECONNRESET, whichis why this only surfaced on Windows.
writeConfig()does not wait for an ack, soclose()runs microseconds aftersendall().Fix
Send FIN first via
shutdown(SHUT_WR), so the device can consume what we wrote,wait briefly for the reader thread to drain and exit, then tear down exactly as
before.
The existing
shutdown(SHUT_RDWR)is deliberately kept: it unblocks a readerstill sitting in
recv()soStreamInterface.close()can join it. The added waitis bounded (
GRACEFUL_CLOSE_TIMEOUT, 0.25s) and returns early when the readerexits, because a device is not obliged to close just because we half-closed.
Why not fix it device-side
It is a race with a tiny window, and the device already loses it. Bisecting the
linger between
writeConfig()andclose(), same device and command:UNSET- lost50ms suffices. meshtasticd polls its socket every 5ms and drains in a loop, so it
is not slow - the RST simply beats its next read. Nothing on the device can read
faster than the gap between the client's
send()and itsclose().Testing
pytest meshtastic/tests/test_tcp_interface.py- 9 passed.asserts
close()survives a peer that has already vanished. The ordering testfails if
close()is reverted to its previous behaviour, so it pins theregression rather than just describing it.
command both times:
lora.region : 0 (UNSET)- lostlora.region : 1 (US)- appliedBehaviour on Linux/macOS is unchanged apart from up to 0.25s of extra latency on
close, and less of it in practice since the wait ends as soon as the reader exits.
Summary by CodeRabbit
Bug Fixes
Tests