Skip to content

Fix TCP read buffering bug breaking ping-based device discovery - #165

Open
imwithsam wants to merge 1 commit into
nikshriv:mainfrom
imwithsam:fix/tcp-read-buffering-and-exception-logging
Open

Fix TCP read buffering bug breaking ping-based device discovery#165
imwithsam wants to merge 1 commit into
nikshriv:mainfrom
imwithsam:fix/tcp-read-buffering-and-exception-logging

Conversation

@imwithsam

Copy link
Copy Markdown

Summary

_read_tcp_messages() processed each reader.read(1000) call in complete isolation, with no buffering across reads. Some packet types — notably the ping-ack response (packet_type 171, ~1019 bytes) — can exceed a single read's worth of bytes over a real network connection, so a packet split across two reads had its leftover bytes silently discarded (data[packet_length+5:] on a too-short buffer just returns empty, no error).

In practice this reliably broke ping-based device discovery (_update_connected_devices) on a fresh connection: the underlying TCP connection was completely healthy (explicit commands worked instantly), but discovery would exhaust all 10 attempts and find zero connected devices every time, leaving every light entity stuck on blank/default state until something else happened to command it directly. Smaller packet types worked fine and masked the issue.

Fixed by accumulating a persistent buffer across read() calls and only parsing a packet once its full declared length has actually arrived, instead of treating a short read as corrupt data to discard.

Also included, found while debugging the above:

  • The initial login-response read had no timeout — a non-responding server could hang the connection forever with no exception, no retry, no log output.
  • Several exception handlers only logged _LOGGER.error(e), which prints an empty line for exception types whose str() is empty, making real failures silent and undiagnosable. Switched to _LOGGER.exception(...) with a descriptive message so a full traceback is always captured.
  • _update_state() silently skipped sending a state request for any home where discovery came back empty. It now falls back to a known controller for that home instead of skipping, as defense-in-depth on top of the actual fix above.

Verification

Reproduced and verified live against a real Cync account/mesh with debug logging enabled:

Before, every ping-ack packet truncated identically:

first time seeing packet_type=171 (0xab), packet_length=1019, actual_len=995
packet_length integrity check FAILED for packet_type=171 -- header says 1019, actual slice is 995

... repeating for the full discovery window, ending with:

ping discovery finished after 10 attempt(s); connected_devices = {'<home_id>': []}

After the fix, on the same account/mesh:

first time seeing packet_type=171 (0xab), packet_length=1019, actual_len=1019
packet_type=171 (ping ack) received for switch_id=..., home_id=...
(x6, once per real device)
ping discovery finished after 1 attempt(s); connected_devices = {'<home_id>': [6 real device ids]}

All devices discovered correctly on the first attempt, and light entities populated with real state on their own with no explicit command needed.

- _read_tcp_messages() processed each socket read(1000) call in
  isolation with no buffering across reads. Some packet types
  (e.g. the ping-ack response, packet_type 171, ~1019 bytes) can
  exceed a single read's worth of bytes over a real network
  connection, so a packet split across two reads had its leftover
  bytes silently discarded. This reliably breaks ping-based device
  discovery (_update_connected_devices) on a fresh connection,
  since its ack packets are large enough to routinely span two
  reads, while smaller packet types happened to work fine and mask
  the issue. Fixed by accumulating a persistent buffer across
  reads and only parsing a packet once its full declared length has
  actually arrived.

- The initial login-response read had no timeout, so a
  non-responding server could hang the connection forever with no
  exception and no retry.

- Several exception handlers logged only `_LOGGER.error(e)`, which
  prints an empty line for exception types whose str() is empty
  (several asyncio/connection-related exceptions), making failures
  silent and undiagnosable. Switched to `_LOGGER.exception(...)`
  with a descriptive message so a full traceback is always logged.

- _update_state() silently skipped sending a state request for any
  home where ping-based discovery came back empty, leaving every
  entity for that home on blank/default data. It now falls back to
  a known controller for the home instead of skipping.

Verified live against a real Cync account/mesh with debug logging:
before the buffering fix, ping-ack packets consistently truncated
from 1019 to 995 bytes and discovery found 0 devices on every cold
start; after the fix, all devices are found in a single discovery
attempt.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant