[Darwin] MTRDevice _deviceMayBeReachable should not reset active subscription

jtung-apple · jtung-apple · commit 4a4cdf7b8b9f · 2025-02-12T10:37:22.000-08:00
diff --git a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm
@@ -336,6 +336,9 @@ @interface MTRDevice_Concrete ()
 @property (nonatomic) NSDate * lastDeviceBecameActiveCallbackTime;
 @property (nonatomic) BOOL throttlingDeviceBecameActiveCallbacks;
 
+// Keep track of the last time we received subscription related communication from the device
+@property (nonatomic) NSDate * lastSubscriptionActiveTime;
+
 /**
  * If currentReadClient is non-null, that means that we successfully
  * called SendAutoResubscribeRequest on the ReadClient and have not yet gotten
@@ -360,6 +363,7 @@ - (void)unitTestSubscriptionPoolWorkComplete:(MTRDevice *)device;
 - (void)unitTestClusterDataPersisted:(MTRDevice *)device;
 - (BOOL)unitTestSuppressTimeBasedReachabilityChanges:(MTRDevice *)device;
 - (void)unitTestSubscriptionCallbackDeleteForDevice:(MTRDevice *)device;
+- (void)unitTestSubscriptionResetForDevice:(MTRDevice *)device;
 @end
 #endif
 
@@ -1697,6 +1701,7 @@ - (void)_handleReportBegin
     assertChipStackLockedByCurrentThread();
 
     std::lock_guard lock(_lock);
+    self.lastSubscriptionActiveTime = [NSDate now];
 
     _receivingReport = YES;
     if (_state != MTRDeviceStateReachable) {
@@ -2669,6 +2674,14 @@ - (void)_resetSubscription
     [self.matterCPPObjectsHolder clearReadClientAndDeleteSubscriptionCallback];
 
     [self _doHandleSubscriptionError:nil];
+
+#ifdef DEBUG
+    [self _callFirstDelegateSynchronouslyWithBlock:^(id testDelegate) {
+        if ([testDelegate respondsToSelector:@selector(unitTestSubscriptionResetForDevice:)]) {
+            [testDelegate unitTestSubscriptionResetForDevice:self];
+        }
+    }];
+#endif
 }
 
 #ifdef DEBUG
@@ -2774,6 +2787,10 @@ - (void)_setupSubscriptionWithReason:(NSString *)reason
                                ^(NSArray * value) {
                                    mtr_strongify(self);
                                    VerifyOrReturn(self, MTR_LOG_DEBUG("_setupSubscriptionWithReason subscription attribute report called back with nil MTRDevice"));
+                                   {
+                                       std::lock_guard lock(self->_lock);
+                                       self.lastSubscriptionActiveTime = [NSDate now];
+                                   }
 
                                    MTR_LOG("%@ got attribute report (%p) %@", self, value, value);
                                    dispatch_async(self.queue, ^{
@@ -2790,6 +2807,10 @@ - (void)_setupSubscriptionWithReason:(NSString *)reason
                                ^(NSArray * value) {
                                    mtr_strongify(self);
                                    VerifyOrReturn(self, MTR_LOG_DEBUG("_setupSubscriptionWithReason subscription event report called back with nil MTRDevice"));
+                                   {
+                                       std::lock_guard lock(self->_lock);
+                                       self.lastSubscriptionActiveTime = [NSDate now];
+                                   }
 
                                    MTR_LOG("%@ got event report %@", self, value);
                                    dispatch_async(self.queue, ^{
@@ -2822,6 +2843,7 @@ - (void)_setupSubscriptionWithReason:(NSString *)reason
 
                                    MTR_LOG("%@ got subscription established", self);
                                    std::lock_guard lock(self->_lock);
+                                   self.lastSubscriptionActiveTime = [NSDate now];
 
                                    // First synchronously change state
                                    if (HadSubscriptionEstablishedOnce(self->_internalDeviceState)) {
@@ -4723,8 +4745,21 @@ - (BOOL)_deviceHasActiveSubscription
     return HaveSubscriptionEstablishedRightNow(_internalDeviceState);
 }
 
+// TODO: make this configurable - for now use 500ms or 0.5 seconds
+#define MTRDEVICE_ACTIVE_COMMUNICATION_THRESHOLD_SECONDS (0.5)
+
 - (void)_deviceMayBeReachable
 {
+    // Ignore this call if actively receiving communication from this device
+    {
+        std::lock_guard lock(self->_lock);
+        NSTimeInterval intervalSinceDeviceLastActive = -[self.lastSubscriptionActiveTime timeIntervalSinceNow];
+        if (intervalSinceDeviceLastActive < MTRDEVICE_ACTIVE_COMMUNICATION_THRESHOLD_SECONDS) {
+            MTR_LOG("%@ _deviceMayBeReachable called and ignored, because last received communication from device %.6lf seconds ago", self, intervalSinceDeviceLastActive);
+            return;
+        }
+    }
+
     MTR_LOG("%@ _deviceMayBeReachable called, resetting subscription", self);
     // TODO: This should only be allowed for thread devices
     mtr_weakify(self);
diff --git a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m
@@ -3691,4 +3691,94 @@ - (void)testMTRDeviceDealloc
     XCTAssertFalse([controller isRunning]);
 }
 
+- (void)testMTRDeviceMaybeUnreachableIgnoredIfReceivingFromDevice
+{
+    __auto_type * storageDelegate = [[MTRTestPerControllerStorageWithBulkReadWrite alloc] initWithControllerID:[NSUUID UUID]];
+
+    __auto_type * factory = [MTRDeviceControllerFactory sharedInstance];
+    XCTAssertNotNil(factory);
+
+    __auto_type queue = dispatch_queue_create("test.queue", DISPATCH_QUEUE_SERIAL_WITH_AUTORELEASE_POOL);
+
+    __auto_type * rootKeys = [[MTRTestKeys alloc] init];
+    XCTAssertNotNil(rootKeys);
+
+    __auto_type * operationalKeys = [[MTRTestKeys alloc] init];
+    XCTAssertNotNil(operationalKeys);
+
+    NSNumber * nodeID = @(333);
+    NSNumber * fabricID = @(444);
+
+    NSError * error;
+
+    MTRPerControllerStorageTestsCertificateIssuer * certificateIssuer;
+    MTRDeviceController * controller = [self startControllerWithRootKeys:rootKeys
+                                                         operationalKeys:operationalKeys
+                                                                fabricID:fabricID
+                                                                  nodeID:nodeID
+                                                                 storage:storageDelegate
+                                                                   error:&error
+                                                       certificateIssuer:&certificateIssuer];
+    XCTAssertNil(error);
+    XCTAssertNotNil(controller);
+    XCTAssertTrue([controller isRunning]);
+
+    XCTAssertEqualObjects(controller.controllerNodeID, nodeID);
+
+    // Now commission the device, to test that that works.
+    NSNumber * deviceID = @(22);
+    certificateIssuer.nextNodeID = deviceID;
+    [self commissionWithController:controller newNodeID:deviceID];
+
+    // We should have established CASE using our operational key.
+    XCTAssertEqual(operationalKeys.signatureCount, 1);
+
+    __auto_type * device = [MTRDevice deviceWithNodeID:deviceID controller:controller];
+    __auto_type * delegate = [[MTRDeviceTestDelegate alloc] init];
+
+    XCTestExpectation * subscriptionReportBegin = [self expectationWithDescription:@"Subscription report begin"];
+    XCTestExpectation * subscriptionReportEnd = [self expectationWithDescription:@"Subscription report end"];
+
+    __weak __auto_type weakDelegate = delegate;
+    delegate.onReportBegin = ^{
+        [subscriptionReportBegin fulfill];
+        __strong __auto_type strongDelegate = weakDelegate;
+        strongDelegate.onReportBegin = nil;
+    };
+
+    delegate.onReportEnd = ^{
+        [subscriptionReportEnd fulfill];
+        __strong __auto_type strongDelegate = weakDelegate;
+        strongDelegate.onReportEnd = nil;
+    };
+
+    delegate.onSubscriptionReset = ^{
+        XCTFail("Subscription should not be reset from calling _deviceMayBeReachable");
+    };
+
+    [device setDelegate:delegate queue:queue];
+
+    // First wait for report to begin coming in
+    [self waitForExpectations:@[ subscriptionReportBegin ] timeout:60];
+
+    // Call _deviceMayBeReachable and expect it to be ignored
+    [device _deviceMayBeReachable];
+
+    [self waitForExpectations:@[ subscriptionReportEnd ] timeout:60];
+
+    [device _deviceMayBeReachable];
+
+    // Since subscription reset runs on the matter queue, synchronously run a block on the matter queue here to prove the reset did not happen
+    [controller syncRunOnWorkQueue:^{
+        ;
+    } error:nil];
+
+    // Reset our commissionee.
+    __auto_type * baseDevice = [MTRBaseDevice deviceWithNodeID:deviceID controller:controller];
+    ResetCommissionee(baseDevice, queue, self, kTimeoutInSeconds);
+
+    [controller shutdown];
+    XCTAssertFalse([controller isRunning]);
+}
+
 @end
diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRDeviceTestDelegate.h b/src/darwin/Framework/CHIPTests/TestHelpers/MTRDeviceTestDelegate.h
@@ -37,6 +37,7 @@ typedef void (^MTRDeviceTestDelegateDataHandler)(NSArray<NSDictionary<NSString *
 @property (nonatomic, nullable) dispatch_block_t onSubscriptionPoolWorkComplete;
 @property (nonatomic, nullable) dispatch_block_t onClusterDataPersisted;
 @property (nonatomic, nullable) dispatch_block_t onSubscriptionCallbackDelete;
+@property (nonatomic, nullable) dispatch_block_t onSubscriptionReset;
 @end
 
 @interface MTRDeviceTestDelegateWithSubscriptionSetupOverride : MTRDeviceTestDelegate
diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRDeviceTestDelegate.m b/src/darwin/Framework/CHIPTests/TestHelpers/MTRDeviceTestDelegate.m
@@ -126,6 +126,13 @@ - (void)unitTestSubscriptionCallbackDeleteForDevice:(MTRDevice *)device
     }
 }
 
+- (void)unitTestSubscriptionResetForDevice:(MTRDevice *)device
+{
+    if (self.onSubscriptionReset != nil) {
+        self.onSubscriptionReset();
+    }
+}
+
 @end
 
 @implementation MTRDeviceTestDelegateWithSubscriptionSetupOverride
diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h
@@ -51,12 +51,14 @@ NS_ASSUME_NONNULL_BEGIN
 @interface MTRDeviceController (Test)
 + (void)forceLocalhostAdvertisingOnly;
 - (void)removeDevice:(MTRDevice *)device;
+- (void)syncRunOnWorkQueue:(void (^)(void))block error:(NSError * __autoreleasing *)error;
 @property (nonatomic, readonly, nullable) id<MTRDeviceControllerDataStoreAttributeStoreMethods> controllerDataStore;
 @end
 
 @interface MTRDevice (Test)
 - (NSMutableArray<NSNumber *> *)arrayOfNumbersFromAttributeValue:(MTRDeviceDataValueDictionary)dataDictionary;
 - (void)setStorageBehaviorConfiguration:(MTRDeviceStorageBehaviorConfiguration *)storageBehaviorConfiguration;
+- (void)_deviceMayBeReachable;
 @end
 
 #pragma mark - Declarations for items compiled only for DEBUG configuration

Original file line number	Diff line number	Diff line change
`@@ -126,6 +126,13 @@ - (void)unitTestSubscriptionCallbackDeleteForDevice:(MTRDevice *)device`
`126`	`126`	`}`
`127`	`127`	`}`
`128`	`128`
	`129`	`+- (void)unitTestSubscriptionResetForDevice:(MTRDevice *)device`
	`130`	`+{`
	`131`	`+ if (self.onSubscriptionReset != nil) {`
	`132`	`+ self.onSubscriptionReset();`
	`133`	`+ }`
	`134`	`+}`
	`135`	`+`
`129`	`136`	`@end`
`130`	`137`
`131`	`138`	`@implementation MTRDeviceTestDelegateWithSubscriptionSetupOverride`