fix(cluster): Bound the readAttributes retry loop

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;

test/readAttributes.js

@@ -0,0 +1,67 @@
+'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 { createSingleClusterDevice } = require('./util/virtualDevice');
+
+const ON_OFF = OnOffCluster.ATTRIBUTES.onOff.id;
+const ON_TIME = OnOffCluster.ATTRIBUTES.onTime.id;
+
+/** 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 }) {
+        const buf = Buffer.alloc(255);
+        const len = OnOffCluster.attributeArrayStatusDataType.toBuffer(buf, answer(attributes), 0);
+        return { attributes: buf.subarray(0, len) };
+      }
+    })(),
+  );
+}
+
+// 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 { zclNode } = deviceAnswering((ids) => {
+      requests.push([...ids]);
+      // 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 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('keeps attributes that were not asked for out of the result', async function () {
+    const { zclNode } = deviceAnswering(() => [
+      { id: ON_OFF, status: 'SUCCESS', value: true },
+      { id: ON_TIME, status: 'SUCCESS', value: 5 },
+    ]);
+
+    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 { zclNode } = deviceAnswering(() => {
+      reads++;
+      return [];
+    });
+
+    const result = await zclNode.endpoints[1].clusters.onOff.readAttributes(['onOff']);
+
+    assert.strictEqual(reads, 1);
+    assert.deepStrictEqual(result, {});
+  });
+});

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',