Skip to content

fix(tcp): prevent select error watchdog loop (EPROT-79) - #183

Open
zyk6271 wants to merge 3 commits into
espressif:mainfrom
zyk6271:agent/prevent-tcp-select-watchdog-loop
Open

zyk6271 wants to merge 3 commits into
espressif:mainfrom
zyk6271:agent/prevent-tcp-select-watchdog-loop

Conversation

@zyk6271

@zyk6271 zyk6271 commented Aug 12, 2026

Copy link
Copy Markdown

Summary

  • validate descriptors before adding them to an fd_set
  • monitor exceptional conditions only for connected sockets and route them through the normal connection error path
  • add bounded exponential backoff and actionable errno logging when select() repeatedly fails
  • release every accepted socket and copied address string when the client table is full or node creation fails
  • recover stale TCP slave clients after fatal lwIP/network errors without deleting persistent TCP master node definitions
  • make close-all transaction cleanup and sparse-node timeout scanning deterministic

Root cause

Two failure modes combined in the reported long-running failure:

  1. The TCP slave accepted a socket and allocated its address string before checking the five-node limit. The rejection path closed the socket but did not release the copied address, so repeated connection attempts steadily exhausted internal heap. Under heap pressure, accept()/select() returned ENOMEM, Wi-Fi allocation failed, and connectivity was lost.
  2. The driver retried select() immediately on persistent errors. The high-priority mb_drv_tcp_task could therefore starve CPU0's IDLE task and trigger the task watchdog.

The previous code also copied the full read set into exceptfds, including non-socket descriptors, and did not consume exceptional client conditions.

Changes

  • check for a free virtual descriptor before allocating a node
  • centralize cleanup of rejected accepted sockets and address metadata
  • preserve accept() errno, avoid close(-1), and handle address/strdup() failures
  • close all stale slave clients on fatal network-stack errors, then purge their transactions
  • keep TCP master node descriptors intact so their reconnect configuration is not lost
  • use the virtual node index, never a negative socket descriptor, with open_set
  • scan all virtual descriptors during timeout processing so holes cannot skip active nodes
  • apply a 1-tick to 1-second bounded exponential backoff to repeated select() failures

Hardware validation

Target: ESP32-D0WD-V3, ESP-IDF v6.0.2, esp-modbus v2.1.3 component sources, Modbus TCP slave with five client slots.

A/B resource-pressure fault injection

The fault injector holds all five clients open, then repeatedly creates extra TCP connections.

Unpatched firmware:

overflow=100   free_internal=91836
overflow=800   free_internal=76084
overflow=2300  free_internal=4202
overflow=3600  free_internal=1264
held clients lost at approximately 3700 overflows
E (...) mb_driver: task select error, errno=12 (Not enough space)
W (...) wifi:mem fail
W (...) wifi:m f null
E (...) task_wdt: IDLE0 did not reset; CPU 0: mb_drv_tcp_task

Patched firmware, with the same temporary heap telemetry:

overflow=100   free_internal=94328
overflow=5000  free_internal=94348
PASS: attempts=6000 rejected=5999 admitted=1; service recovered

A fresh run after the final source-review fixes:

holding 5 active client(s)
progress=6000/6000 rejected=6000 admitted=0 held_alive=5
PASS: attempts=6000 rejected=6000 admitted=0; service recovered

Additional fault injection/regression

PASS: node-table holes/refill cycles=100
malformed MBAP/PID/UID/truncated frame cases: 4/4; service alive after each
Wi-Fi disabled for 15 seconds: held clients discarded; original IP and Modbus service recovered

During the patched test windows there was no errno=12, wifi:mem fail, wifi:m f null, select busy loop, or task WDT. A single errno=113 while Wi-Fi was physically unavailable was expected and recovery completed after reassociation.

Build validation

  • rebuilt TCP master, TCP slave, TCP driver, and TCP utility objects
  • linked and flashed the complete application successfully
  • firmware size: 0xfdb30; smallest app partition remained 32% free
  • git diff --check passed for all five changed library files

@zyk6271
zyk6271 marked this pull request as ready for review August 12, 2026 02:13
@github-actions github-actions Bot changed the title fix(tcp): prevent select error watchdog loop fix(tcp): prevent select error watchdog loop (EPROT-79) Aug 12, 2026
@alisitsyn

alisitsyn commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Hi @ zyk6271,

Thanks for the PR proposal. Unfortunately, I don't have time to review and test the related cases for now. I will take a look once have time for this. Please consider to extend your PR with the logs of stress testing with fault injection.

@zyk6271

zyk6271 commented Aug 17, 2026

Copy link
Copy Markdown
Author

Implemented the requested stress/fault-injection follow-up in commit 466cdb57f72f69a52707ba2b574a5939dd2dfc2a and added the complete A/B data to the PR description.

Key results on ESP32-D0WD-V3 / ESP-IDF v6.0.2:

unpatched:
  free_internal: 91836 @ 100 overflows
                 76084 @ 800
                  4202 @ 2300
                  1264 @ 3600
  held clients lost at ~3700, followed by errno=12 / Wi-Fi allocation failures / task WDT

patched:
  free_internal: 94328 @ 100 overflows
                 94348 @ 5000
  PASS: 6000/6000 extra connections rejected, all 5 held clients alive
  PASS: service recovered after held clients were released
  PASS: 100 sparse-node close/refill cycles
  PASS: 4 malformed-frame cases, service alive after each
  PASS: 15-second Wi-Fi outage and Modbus recovery

No errno=12, wifi:mem fail, wifi:m f null, select busy loop, or task WDT was observed in the patched windows.

The follow-up also fixes the accepted-socket/address leak itself, preserves TCP master reconnect definitions during fatal-error recovery, and removes negative-fd FD_ISSET calls in both master and slave close handlers.

@alisitsyn

Copy link
Copy Markdown
Collaborator

@zyk6271 ,

Thanks. for the update. I just reviewed it quickly.

The follow-up also fixes the accepted-socket/address leak itself, preserves TCP master reconnect definitions during fatal-error recovery, and removes negative-fd FD_ISSET calls in both master and slave close handlers.

I have some related fixes in my other opened MR. So, your PR needs to be rebased to main. I will check all aspects later. This PR will follow the formal process and I will inform you about status.

@alisitsyn alisitsyn self-assigned this Aug 27, 2026
Comment thread modbus/mb_ports/tcp/port_tcp_driver.c
Release accepted sockets and address metadata on all failure paths, close
stale slave clients after fatal network-stack errors, and keep the master
node table intact for reconnects. Also fix sparse-node timeout scanning and
avoid using negative socket descriptors as fd_set indexes.
@zyk6271
zyk6271 force-pushed the agent/prevent-tcp-select-watchdog-loop branch from 466cdb5 to 73a67f8 Compare August 28, 2026 00:46
@zyk6271

zyk6271 commented Aug 28, 2026

Copy link
Copy Markdown
Author

Rebased this PR onto the latest main (28e254e).

The two patch commits were preserved unchanged according to git range-diff:

  • b3f7894 fix(tcp): prevent select error watchdog loop
  • 73a67f8 fix(tcp): recover resources under connection pressure

I also reviewed the related changes already present on main. The safe fd-set macro and shared close-all implementation mentioned in the inline notes are not on main yet, so I kept the currently tested behavior in this PR for now. I can rebase and remove any overlap once the related MR lands.

@alisitsyn

Copy link
Copy Markdown
Collaborator

I have a lot of fixes in the separate MRs that are not yet merged into main. You can check and keep your PR with minimal changes. Once possible to continue, I will verify and take your original commit and adopt it with additional fix commit in my branch as required. This will take time as per formal process. I will inform you if I have any further questions or information. Thanks for contribution.

Comment thread modbus/mb_ports/tcp/port_tcp_driver.c Outdated
Comment thread modbus/mb_ports/tcp/port_tcp_driver.c Outdated
Comment thread modbus/mb_ports/tcp/port_tcp_slave.c
@rnavarro

rnavarro commented Sep 16, 2026

Copy link
Copy Markdown

A production Modbus TCP slave of mine stopped answering: port 502 completed the TCP handshake and reset the connection on the first request, and the web server on the same board stopped answering too. Nothing remote could recover it; bouncing its switch port did. I reproduced it on a spare board of the same model, and it is the same select() watchdog loop this PR describes, reached through socket exhaustion instead of heap pressure. This PR fixes it on my hardware.

Setup: Waveshare ESP32-S3-POE-ETH-8DI-8RO (W5500 Ethernet), ESP-IDF 5.5.4, esp-modbus 2.1.3 as a TCP slave, CONFIG_FMB_TCP_PORT_MAX_CONN=5, CONFIG_LWIP_MAX_SOCKETS=10, CONFIG_FMB_PORT_TASK_PRIO=10 pinned to CPU0, plus esp_http_server with max_open_sockets = 4.

Trigger: a host holds 6 HTTP connections and opens 8 Modbus connections at the same time, which runs lwIP out of sockets.

Unpatched 2.1.3 (2 of 2 runs):

E (239505) port.utils: Unable to accept connection: errno=23
E (239505) port.utils: Sock 1, enable keep alive option fail, err= (-1).
E (244545) task_wdt: Task watchdog got triggered. The following tasks/users did not reset the watchdog in time:
E (244545) task_wdt:  - IDLE0 (CPU 0)
E (244545) task_wdt: CPU 0: mb_drv_tcp_task

The backtrace is esp_vfs_select <- mb_drv_wait_fd_events (port_tcp_driver.c:527) <- mb_drv_tcp_task (:609). accept() returns -1 with ENFILE, if (sock_id) passes it, mb_drv_open takes a slot from an uninitialised mb_uid_info_t, descriptor 1 goes into the select set, and from the source the ret == -1 branch then loops with no delay, which matches the backtrace. The task watchdog fires every 5 s from then on. No Modbus request is served, and the uptime counter my main loop on CPU0 updates stops advancing. In one of the two runs the HTTP server stopped answering too. The board stayed like that until an application-level supervisor restarted it after 10 minutes.

A side note on the #127 suggestion: zero-initialising node_info alone gives sock_id 0, which mb_drv_register_fds skips, but if (sock_id) still passes -1. So the node would still take a slot and never release it. This PR changes both, which is what my case needs.

With this PR: I tested two builds on the same board, with the same trigger and application code:

Build Exhaustion rounds accept() failures (errno=23) Free slots after Watchdog / resets
v2.1.3 + b3f7894, 73a67f8, 9256b88 (applied cleanly) 30 180 5 of 5 none
PR head 9256b88 on main 30 180 5 of 5 none

In both builds the uptime kept advancing, and the HTTP server answered throughout. 30 rounds each of connect/read/close, connect/read/RST and connect/close without a request left all 5 slots free. The Sock 1 line and the select error line did not appear. The failed accepts are logged and the loop carries on.

Short test, not a long soak. I did not test a peer that disappears without a FIN or RST, or the heap-pressure path from your description.

@alisitsyn

alisitsyn commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

@rnavarro,

I don't think it is reliable to open maximum number of sockets in the production project with limited resources. However, your stress test results show the error handling issue clearly and are useful. I will consider increasing the priority of this PR to merge it as soon as possible and merge other related MRs with related fixes later.

Setup: Waveshare ESP32-S3-POE-ETH-8DI-8RO (W5500 Ethernet), ESP-IDF 5.5.4, esp-modbus 2.1.3 as a TCP slave, CONFIG_FMB_TCP_PORT_MAX_CONN=5, CONFIG_LWIP_MAX_SOCKETS=10, CONFIG_FMB_PORT_TASK_PRIO=10 pinned to CPU0, plus esp_http_server with max_open_sockets = 4.

Trigger: a host holds 6 HTTP connections and opens 8 Modbus connections at the same time, which runs lwIP out of sockets.

I tried to verify some aspects for this PR and the socket accept limitation for the TCP Slave from LWIP side. It looks like the TCP Slave can not accept more than 7 connections, and resets all following connections, even if CONFIG_FMB_TCP_PORT_MAX_CONN > 7 and CONFIG_LWIP_MAX_SOCKETS > 10. This is limitation related to LWIP and its CONFIG_LWIP_TCP_ACCEPTMBOX_SIZE = 6 set by default. LWIP aborted the handshakes that did not fit into its accept mailbox. This should apply to your system as well.

Thanks for test report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants