Half-close TCP connections so the last write is not lost - #958
Conversation
Sorry, something went wrong.
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
WalkthroughTCPInterface.close() now performs a bounded graceful half-close, waits for the reader thread, and then forces socket cleanup. Tests verify operation ordering and resilience when socket shutdown raises OSError. 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: ianmcorvidae 🚥 Pre-merge checks | ✅ 5 ✅ Passed checks (5 passed)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
There was a problem hiding this comment.
meshtastic/tcp_interface.py (1)🤖 Prompt for all review comments with AI agents117-119: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
Consider 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 a try block.
💡 Proposed optional refactor🤖 Prompt for AI Agents- 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: + passVerify 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.
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
📥 CommitsReviewing files that changed from the base of the PR and between 82220ba and f623732.
📒 Files selected for processing (2)- meshtastic/tcp_interface.py
- meshtastic/tests/test_tcp_interface.py
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.
@@ 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.
|
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
It happened twice in a row now. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Fixes #957.
One-shot config writes over TCP are silently lost on Windows: --set reports
success and the device never applies it. Reads are unaffected.
Cause
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 the admin message written moments earlier is thrown away before the device gets
to read it. Linux delivers the buffered bytes before reporting ECONNRESET, which
is why this only surfaced on Windows.
writeConfig() does not wait for an ack, so close() runs microseconds after
sendall().
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 reader
still sitting in recv() so StreamInterface.close() can join it. The added wait
is bounded (GRACEFUL_CLOSE_TIMEOUT, 0.25s) and returns early when the reader
exits, 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() and close(), same device and command:
50ms 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 its close().
Testing
asserts close() survives a peer that has already vanished. The ordering test
fails if close() is reverted to its previous behaviour, so it pins the
regression rather than just describing it.
command both times:
Behaviour 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