Skip to content

Drop the write response of a setter that answers after its connection closed - #557

Merged
ikalchev merged 1 commit into
devfrom
fix/stale-write-response
Nov 12, 2026
Merged

ikalchev merged 1 commit into
devfrom
fix/stale-write-response

Conversation

@ikalchev

@ikalchev ikalchev commented Nov 12, 2026 •

Copy link
Copy Markdown
Owner

Follow-up to the #544 review: a per-connection write response could outlive its connection.

The deferred cleanup in connection_lost pops a connection's write responses once. A coroutine setter that finished later stored its response again, via setdefault, and nothing removed it: the entry stayed under the closed connection's address forever, where a later connection that reuses the address read it. A Setup Endpoints answer carries its session's SRTP keys, so it must not outlive the connection that wrote.

  • The driver registers each controller write with pending coroutine setters in _pending_connection_writes, and connection_lost marks those writes dead.
  • _async_finish_set_characteristics drops the responses of dead writes and unregisters them, whatever a setter returns. A response is answered once, to the writing connection, so one that answers after the close would never be read anyway.

The existing test covered a completion that was already queued when the connection closed; the new tests cover a setter finishing after the deferred cleanup ran, for both a wr write response and a read_back_response characteristic. Both fail without the fix. The guard is per pending batch, so nothing accumulates for dead connections.

This PR was made with LLM (GLM-5.3). Verifications mine.

… closed

The deferred cleanup in connection_lost pops the per-connection write
responses once. A coroutine setter that finished later stored its
response again, via setdefault, and nothing removed it: it stayed under
the closed connection's address, where a connection that reuses the
address read it. A Setup Endpoints answer carries its session's SRTP
keys, so it must not outlive the connection that wrote.

The driver now marks the pending writes of a connection dead in
connection_lost, and _async_finish_set_characteristics drops the
responses of dead writes. A response is answered once, to the writing
connection, so one that answers after the close would never be read
anyway.
@codecov

codecov Bot commented Nov 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.00%. Comparing base (f7406d3) to head (ea15d57).

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #557   +/-   ##
=======================================
  Coverage   91.99%   92.00%           
=======================================
  Files          50       50           
  Lines       10822    10839   +17     
  Branches     1402     1404    +2     
=======================================
+ Hits         9956     9972   +16     
  Misses        579      579           
- Partials      287      288    +1     
Files with missing lines Coverage Δ
pyhap/accessory_driver.py 94.12% <100.00%> (+0.14%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ikalchev ikalchev self-assigned this Nov 12, 2026
@ikalchev
ikalchev merged commit 412b899 into dev Nov 12, 2026
14 checks passed
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