From bd441be807988340ccb4fa4e0753c1f8a5829aa8 Mon Sep 17 00:00:00 2001 From: Robin Bolscher Date: Tue, 27 Oct 2026 10:46:24 +0100 Subject: [PATCH 1/2] fix(cluster): Bound the readAttributes retry loop 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. --- lib/Cluster.js | 24 +++++++---- test/readAttributes.js | 98 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 8 deletions(-) create mode 100644 test/readAttributes.js diff --git a/lib/Cluster.js b/lib/Cluster.js index d59953f..348028f 100644 --- a/lib/Cluster.js +++ b/lib/Cluster.js @@ -494,17 +494,25 @@ class Cluster extends EventEmitter { debug(this.logId, 'read attributes result', response); const { attributeArrayStatusDataType } = this.constructor; const result = attributeArrayStatusDataType.fromBuffer(response.attributes, 0); - if (!result.length) break; - - result.forEach((a) => { - attrIds.delete(a.id); - if (a.status === 'SUCCESS') { - resultObj[idToName[a.id]] = a.value; - if (numericIds.has(a.id) && idToName[a.id] !== a.id) { - resultObj[a.id] = a.value; + + const remaining = attrIds.size; + result.forEach((record) => { + // Devices do answer with attributes that were not asked for. Dropping those keeps them + // out of the result, where they would land under an undefined key. + if (!attrIds.delete(record.id)) return; + if (record.status === 'SUCCESS') { + resultObj[idToName[record.id]] = record.value; + if (numericIds.has(record.id) && idToName[record.id] !== record.id) { + resultObj[record.id] = record.value; } } }); + + // Asking again would repeat the request that just made no progress. + if (attrIds.size === remaining) { + debug(this.logId, 'read attributes gave up, unanswered:', [...attrIds]); + break; + } } return resultObj; diff --git a/test/readAttributes.js b/test/readAttributes.js new file mode 100644 index 0000000..43705a1 --- /dev/null +++ b/test/readAttributes.js @@ -0,0 +1,98 @@ +'use strict'; + +const { describe, it } = require('node:test'); +const assert = require('node:assert'); + +const BoundCluster = require('../lib/BoundCluster'); +const OnOffCluster = require('../lib/clusters/onOff'); +const { createMockNode } = require('./util'); + +const ON_OFF = OnOffCluster.ATTRIBUTES.onOff.id; +const ON_TIME = OnOffCluster.ATTRIBUTES.onTime.id; + +/** Encode read-attribute status records the way a remote cluster would answer. */ +function encode(records) { + const buf = Buffer.alloc(255); + const len = OnOffCluster.attributeArrayStatusDataType.toBuffer(buf, records, 0); + return buf.slice(0, len); +} + +/** + * Mock node whose bound onOff cluster answers every read with `answer(requestedIds)`, so a test + * can decide what the remote side reports back. + */ +function nodeAnswering(answer) { + const node = createMockNode({ + loopback: true, + endpoints: [{ endpointId: 1, inputClusters: [OnOffCluster.ID] }], + }); + + node.endpoints[1].bind( + 'onOff', + new (class extends BoundCluster { + async readAttributes({ attributes }) { + return { attributes: encode(answer(attributes)) }; + } + })(), + ); + + return node; +} + +describe('Cluster readAttributes', function () { + it('reads what it asked for across several responses', async function () { + const requests = []; + + const node = nodeAnswering((ids) => { + requests.push([...ids]); + // Answer one attribute at a time, as a device splitting a response over several frames does. + const id = ids[0]; + return [{ id, status: 'SUCCESS', value: id === ON_OFF ? true : 5 }]; + }); + + const result = await node.endpoints[1].clusters.onOff.readAttributes(['onOff', 'onTime']); + + assert.deepStrictEqual(requests, [[ON_OFF, ON_TIME], [ON_TIME]]); + assert.deepStrictEqual(result, { onOff: true, onTime: 5 }); + }); + + it('stops asking when a response holds none of the requested attributes', async function () { + let reads = 0; + + const node = nodeAnswering(() => { + reads++; + // Never answers onOff, only an attribute that was not asked for. + return [{ id: ON_TIME, status: 'SUCCESS', value: 5 }]; + }); + + const result = await node.endpoints[1].clusters.onOff.readAttributes(['onOff']); + + assert.strictEqual(reads, 1); + assert.deepStrictEqual(result, {}); + }); + + it('keeps attributes that were not asked for out of the result', async function () { + const node = nodeAnswering(() => [ + { id: ON_OFF, status: 'SUCCESS', value: true }, + { id: ON_TIME, status: 'SUCCESS', value: 5 }, + ]); + + const result = await node.endpoints[1].clusters.onOff.readAttributes(['onOff']); + + assert.deepStrictEqual(result, { onOff: true }); + }); + + it('stops on a response that holds no attributes at all', async function () { + let reads = 0; + + const node = nodeAnswering(() => { + reads++; + return []; + }); + + const result = await node.endpoints[1].clusters.onOff.readAttributes(['onOff']); + + assert.strictEqual(reads, 1); + assert.deepStrictEqual(result, {}); + }); +}); From addf07b2143c59137c0e3c55a2e1140369a3b0da Mon Sep 17 00:00:00 2001 From: Robin Bolscher Date: Tue, 27 Oct 2026 13:44:38 +0100 Subject: [PATCH 2/2] test: Turn the readAttributes known-bug test into a real one 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. --- test/readAttributes.js | 63 +++++++++++------------------------------- test/virtualDevice.js | 48 +++++++++++++++----------------- 2 files changed, 38 insertions(+), 73 deletions(-) diff --git a/test/readAttributes.js b/test/readAttributes.js index 43705a1..24a6728 100644 --- a/test/readAttributes.js +++ b/test/readAttributes.js @@ -5,92 +5,61 @@ const assert = require('node:assert'); const BoundCluster = require('../lib/BoundCluster'); const OnOffCluster = require('../lib/clusters/onOff'); -const { createMockNode } = require('./util'); +const { createSingleClusterDevice } = require('./util/virtualDevice'); const ON_OFF = OnOffCluster.ATTRIBUTES.onOff.id; const ON_TIME = OnOffCluster.ATTRIBUTES.onTime.id; -/** Encode read-attribute status records the way a remote cluster would answer. */ -function encode(records) { - const buf = Buffer.alloc(255); - const len = OnOffCluster.attributeArrayStatusDataType.toBuffer(buf, records, 0); - return buf.slice(0, len); -} - -/** - * Mock node whose bound onOff cluster answers every read with `answer(requestedIds)`, so a test - * can decide what the remote side reports back. - */ -function nodeAnswering(answer) { - const node = createMockNode({ - loopback: true, - endpoints: [{ endpointId: 1, inputClusters: [OnOffCluster.ID] }], - }); - - node.endpoints[1].bind( - 'onOff', +/** A device whose onOff cluster answers every read with the records `answer(requestedIds)`. */ +function deviceAnswering(answer) { + return createSingleClusterDevice( + OnOffCluster, new (class extends BoundCluster { async readAttributes({ attributes }) { - return { attributes: encode(answer(attributes)) }; + const buf = Buffer.alloc(255); + const len = OnOffCluster.attributeArrayStatusDataType.toBuffer(buf, answer(attributes), 0); + return { attributes: buf.subarray(0, len) }; } })(), ); - - return node; } +// The device that only ever answers with unrequested ids is in test/virtualDevice.js. describe('Cluster readAttributes', function () { it('reads what it asked for across several responses', async function () { const requests = []; - - const node = nodeAnswering((ids) => { + const { zclNode } = deviceAnswering((ids) => { requests.push([...ids]); - // Answer one attribute at a time, as a device splitting a response over several frames does. + // One attribute per response, as a device splitting its answer over several frames does. const id = ids[0]; return [{ id, status: 'SUCCESS', value: id === ON_OFF ? true : 5 }]; }); - const result = await node.endpoints[1].clusters.onOff.readAttributes(['onOff', 'onTime']); + const result = await zclNode.endpoints[1].clusters.onOff.readAttributes(['onOff', 'onTime']); assert.deepStrictEqual(requests, [[ON_OFF, ON_TIME], [ON_TIME]]); assert.deepStrictEqual(result, { onOff: true, onTime: 5 }); }); - it('stops asking when a response holds none of the requested attributes', async function () { - let reads = 0; - - const node = nodeAnswering(() => { - reads++; - // Never answers onOff, only an attribute that was not asked for. - return [{ id: ON_TIME, status: 'SUCCESS', value: 5 }]; - }); - - const result = await node.endpoints[1].clusters.onOff.readAttributes(['onOff']); - - assert.strictEqual(reads, 1); - assert.deepStrictEqual(result, {}); - }); - it('keeps attributes that were not asked for out of the result', async function () { - const node = nodeAnswering(() => [ + const { zclNode } = deviceAnswering(() => [ { id: ON_OFF, status: 'SUCCESS', value: true }, { id: ON_TIME, status: 'SUCCESS', value: 5 }, ]); - const result = await node.endpoints[1].clusters.onOff.readAttributes(['onOff']); + const result = await zclNode.endpoints[1].clusters.onOff.readAttributes(['onOff']); assert.deepStrictEqual(result, { onOff: true }); }); it('stops on a response that holds no attributes at all', async function () { let reads = 0; - - const node = nodeAnswering(() => { + const { zclNode } = deviceAnswering(() => { reads++; return []; }); - const result = await node.endpoints[1].clusters.onOff.readAttributes(['onOff']); + const result = await zclNode.endpoints[1].clusters.onOff.readAttributes(['onOff']); assert.strictEqual(reads, 1); assert.deepStrictEqual(result, {}); diff --git a/test/virtualDevice.js b/test/virtualDevice.js index e9f19f8..436e68e 100644 --- a/test/virtualDevice.js +++ b/test/virtualDevice.js @@ -97,34 +97,30 @@ describe('Virtual device', function () { }); describe('Virtual device: known bugs on master', function () { - it( - 'readAttributes stops when the device answers with unrequested ids', - { todo: 'fixed by #258' }, - async function () { - // Whatever is asked, the plug answers with onTime only - const { zclNode, intercept } = createSingleClusterDevice( - OnOffCluster, - new (class extends BoundCluster { - async readAttributes() { - const buf = Buffer.alloc(255); - const records = [{ id: ON_TIME, status: 'SUCCESS', value: 5 }]; - const len = OnOffCluster.attributeArrayStatusDataType.toBuffer(buf, records, 0); - return { attributes: buf.subarray(0, len) }; - } - })(), - ); - let reads = 0; - intercept((frame) => { - // Guard: on master the read loops forever. - if (frame.from === 'zclNode' && ++reads > 3) throw new Error('read loop'); - }); + it('readAttributes stops when the device answers with unrequested ids', async function () { + // Whatever is asked, the plug answers with onTime only + const { zclNode, intercept } = createSingleClusterDevice( + OnOffCluster, + new (class extends BoundCluster { + async readAttributes() { + const buf = Buffer.alloc(255); + const records = [{ id: ON_TIME, status: 'SUCCESS', value: 5 }]; + const len = OnOffCluster.attributeArrayStatusDataType.toBuffer(buf, records, 0); + return { attributes: buf.subarray(0, len) }; + } + })(), + ); + let reads = 0; + intercept((frame) => { + // Guard: on master the read loops forever. + if (frame.from === 'zclNode' && ++reads > 3) throw new Error('read loop'); + }); - const result = await zclNode.endpoints[1].clusters.onOff.readAttributes(['onOff']); + const result = await zclNode.endpoints[1].clusters.onOff.readAttributes(['onOff']); - assert.strictEqual(reads, 1); - assert.deepStrictEqual(result, {}); - }, - ); + assert.strictEqual(reads, 1); + assert.deepStrictEqual(result, {}); + }); it( 'readAttributes([]) on metering splits the request to fit a frame',