Repository navigation
fix(cluster): Bound the readAttributes retry loop - #258
Merged
Merged
Conversation
RobinBol
changed the title
RobinBol
force-pushed
the
fix/read-attributes-retry-loop
branch
from
October 27, 2026 10:05
047c017 to
054f41c
Compare
RobinBol
marked this pull request as ready for review
October 27, 2026 10:07
RobinBol
added a commit
that referenced
this pull request
Oct 27, 2026
* test: Add a virtual device harness for misbehaving-device edge cases Real devices behave too well to reproduce bugs like the readAttributes retry loop. createVirtualDevice puts the device on its own node behind an in-memory link that records every frame, enforces Homey's 82-byte ZCL frame limit, and lets a test drop, rewrite or inject frames. test/virtualDevice.js keeps four bugs that reproduce on master as todo tests: the readAttributes loop on unrequested ids (#258), readAttributes([]) on metering overflowing a frame (#259), Endpoint.unbind leaving a Cluster in bindings, and attr.undefined emitted for unknown ids in a report. Refs #258 Refs #259 * test: Move every test onto the virtual device harness The suite had four ways to fake a node: createMockNode, createConnectedNodePair, MOCK_DEVICES and hand-built Node stubs. All of them are now createVirtualDevice, and test/util/mockNode.js is gone. Migrating exposed tests that could not fail. They now can, each checked by breaking the library code it covers: - waitForResponse: false was passed as command args, so the test waited anyway. - The mixed-manufacturer tests accepted any error, including one from a binding left over from an earlier test. - Not answering a default response was never reached, because our own default responses set Disable Default Response; the device now sends one without it. - Assertions inside handlers became error responses or hung done callbacks; tests now assert after await or settle(), and on wire frames instead of spies. - The attribute 0x0000 test asserted nothing; it now decodes a noData report. Duplicates of the ZCLError and Default Response tests are removed, the four tests of MOCK_DEVICES itself are dropped, and OTA runs the way Homey uses it: the device requests, the node serves. The harness gains frame meta, eager parsing and a 1000-frame cap that turns a request/response loop into a failure. * docs: Document the virtual device test harness docs/testing.md explains both sides of createVirtualDevice, its options and return values, recipes for misbehaving devices, and the rules that keep a test able to fail. AGENTS.md and the README point to it. * test: Turn the Xiaomi lifeline script into a real test test/manual/xiaomiHacks.js registered a Basic cluster that declares its own data type for Xiaomi's attribute 0xff01, but never decoded a frame, so it checked nothing. The new test has the virtual device send a report shaped like the ones Xiaomi devices send and asserts the decoded records. It fails when the library decodes with the type on the wire instead of the declared one, which is what such a custom cluster relies on. * test: Deliver harness frames after sendFrame resolves Frames were delivered while sendFrame was still running, so every reply arrived before the command armed its response timeout: setting that timeout to 0 failed one test. Frames now arrive on the next turn of the event loop, as on Homey where sendFrame resolves on the APS ACK and the response comes later. The same mutation now fails 33 tests. A loop between two handlers only debug-logged the frame cap error, so the test passed. settle() now rethrows a failed delivery, including the cap. An interceptor that returns a non-Buffer throws instead of being dropped silently. frameToClient() and createSingleClusterDevice() replace the hand-built headers and per-file single-cluster helpers. frames[].error, which the node never let through, is gone, and docs/testing.md matches the code. * test: Import the harness directly instead of through test/util/index.js The index only re-exported test/util/virtualDevice.js. docs/adding-clusters.md still pointed at the removed mock node helpers there; it now links the testing guide. * docs: Cut the testing guide to what the code does not already say The diagram repeated the bullets under it, frameToClient was explained three times, and the misbehaving-device snippet was plain BoundCluster use. * docs: Explain the test harness as a concept and point to examples Option and return value details now live as JSDoc on createVirtualDevice, next to the code they describe, so they cannot drift from it. The guide keeps the idea, a table of example tests per scenario, and the rules.
A record for an attribute that was not requested has no entry in the name map, so it was written into the result under the key `undefined`. Drop such records instead. The same check bounds the loop. readAttributes re-asks for whatever the remote cluster has not answered yet, which is what lets a device split a response over several frames. An attribute left the pending set only when the device reported it, so a response holding none of the requested ids left the set unchanged and the next pass sent a byte-identical request, for as long as the process ran. No device was seen doing this and no issue reports it: the loop simply had no cap and no deadline, and no timeout fires because each individual read succeeds. Stop once a pass removes nothing, logging what went unanswered. A response carrying two records for the same attribute now keeps the first rather than the last, because the second no longer matches a pending id.
The harness carries the device that only ever answers with unrequested ids as a todo test for this fix, so the todo goes. The same case in test/readAttributes.js was a duplicate and is dropped; the rest of that file now runs on the harness instead of the removed mock node.
RobinBol
force-pushed
the
fix/read-attributes-retry-loop
branch
from
October 27, 2026 12:46
054f41c to
addf07b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Cluster.readAttributesdrops records for attributes it did not ask for, and stops re-sending itsread once a pass removes nothing from the pending set.
Why
Such a record has no entry in the name map, so it was stored under the key
undefined. It also leftthe pending set unchanged, so the next pass sent a byte-identical request, with no cap on the loop.
No device was seen doing this. The virtual device harness reproduces it: its known-bug test fails on
masterand passes here. On an IKEA INSPELNING plug, 45 reads gave identical results on both andnever an unrequested record. It bounds a loop that had no bound.
How
attrIds.delete()reports whether the id was pending, so one check does both:A response with two records for one attribute now keeps the first instead of the last.