[MTRCommissioning] Add watchdog for client failure to advance past PASE

If a client (CHIP framework consumer) implements
commissioning:paseSessionEstablishmentComplete: but then never calls
commissionNodeWithID: or cancelCommissioningForNodeID:, the
MTRCommissioningOperation sits in isWaitingAfterPASEEstablished = YES
forever.  MTRDeviceController_Concrete keeps the operation alive via
currentInternalCommissioning, so every subsequent commissioning attempt
hits the busy check in startWithController and fails with
CHIP_ERROR_BUSY.  User-visible symptom: Matter accessories refuse to
commission until the daemon (or app) is restarted.

Add a 5-minute dispatch_source_t watchdog armed when
isWaitingAfterPASEEstablished is set to YES, cancelled on legitimate
advance, stop, or terminal error.  On expiry, calls
stopCommissioning:forCommissioningID: and routes CHIP_ERROR_TIMEOUT
through the standard _dispatchCommissioningError path so client state
can settle.

Adds a defensive secondary fix in
setupCommissioningSessionWithPayload:newNodeID:error:: if a stale
internally-created commissioning is still parked at the post-PASE
waiting state with a different setup payload, treat the new call as an
implicit cancel of the previous one rather than failing the new call
with BUSY.

Threading hardening from review:
 * atomic test-only watchdog interval override so concurrent TSan setters
   on any queue race-free with the production read in _armPostPASEWatchdog
 * atomic stop via stopCommissioningAtomically:forCommissioningID:, closing
   the TOCTOU window between currentCommissioning read and StopPairing
 * synchronous _cancelPostPASEWatchdog in -stop so tests can observe the
   teardown immediately after -stop returns

Honor the @Property (copy) declaration on -setupPayload by [copy]'ing
the input in the designated initializer; mutable inputs were aliased.

Includes regression coverage for the watchdog arm/cancel/fire paths,
late-fire guard, one-shot semantics, queue serialization, the
implicit-cancel and same-payload-no-cancel behaviors of
setupCommissioningSessionWithPayload:, plus edge cases: test-only
interval reset-to-zero restoring the production interval, a TSan smoke
test for the per-instance force-arm-failure flag, and the synchronous
self-fire watchdog cancel contract.

Author: woody-apple

src/darwin/Framework/CHIP/MTRCommissioningOperation.mm

@@ -29,13 +29,41 @@
 #import "MTRMetricsCollector.h"
 #import "MTRMetrics_Internal.h"
 
+#if DEBUG
+#import "MTRCommissioningOperation_Test.h"
+#endif
+
+#include <math.h>
+#include <stdatomic.h>
+
 #include <controller/ExampleOperationalCredentialsIssuer.h>
 #include <lib/core/CHIPError.h>
 #include <setup_payload/SetupPayload.h>
 
 using namespace chip;
 using namespace chip::Tracing::DarwinFramework;
 
+// Interval after which the post-PASE watchdog fires.  The watchdog releases the
+// controller's single in-flight commissioning slot if a client establishes PASE
+// (and asks to be notified via paseSessionEstablishmentComplete:) but then never
+// calls commissionNodeWithID: or cancelCommissioningForNodeID:.  Without it,
+// currentCommissioning is never cleared and every subsequent commissioning
+// attempt fails with CHIP_ERROR_BUSY (0xDB) until the process restarts.
+//
+// Five minutes is well past the commissionee fail-safe (60-180s): the watchdog
+// is not racing the on-device handshake (the fail-safe handles that), it only
+// reclaims our local slot for a client that has gone away, so it is generous
+// enough not to tear a slow-but-live client out from under the user.
+static const NSTimeInterval kMTRPostPASEWatchdogInterval = 5 * 60;
+
+#if DEBUG
+// Test-only override for kMTRPostPASEWatchdogInterval so tests can exercise the
+// watchdog without waiting five minutes.  Atomic because the test setter may run
+// on a different thread than the _delegateQueue read in -_armPostPASEWatchdog.
+// Only compiled into DEBUG (test) builds; never ships in the release framework.
+static _Atomic(NSTimeInterval) sMTRPostPASEWatchdogIntervalForTesting = 0;
+#endif
+
 @interface MTRCommissioningOperationDeviceAttestationDelegate : NSObject <MTRDeviceAttestationDelegate>
 @property (nonatomic, weak) MTRCommissioningOperation * commissioningOperation;
 @end
@@ -49,6 +77,41 @@ @implementation MTRCommissioningOperation {
     id<MTRCommissioningDelegate> __weak _delegate;
     dispatch_queue_t _delegateQueue;
     MTRDeviceController_Concrete * __weak _controller;
+    // Watchdog timer armed once PASE has been established to bound how long we
+    // hold the controller in the "in-progress commissioning" state if the
+    // client drops the flow without ever calling commissionNodeWithID: or
+    // cancelCommissioningForNodeID:.  Without this watchdog,
+    // currentCommissioning is never cleared and every subsequent commissioning
+    // attempt hits CHIP_ERROR_BUSY (0xDB) until the process restarts.
+    //
+    // _postPASEWatchdog is only ever created, cancelled, or cleared on
+    // _delegateQueue (the queue all delegate callbacks and the timer's
+    // event_handler run on), so it needs no separate lock.
+    dispatch_source_t _postPASEWatchdog;
+    // Backing storage for isWaitingAfterPASEEstablished.  Accessed via C11
+    // atomics because it is read and written from arbitrary client queues
+    // (MTRDeviceController_Concrete maintains it) as well as _delegateQueue.
+    _Atomic(BOOL) _isWaitingAfterPASEEstablished;
+}
+
+@synthesize commissioningID = _commissioningID;
+
+- (BOOL)isWaitingAfterPASEEstablished
+{
+    return atomic_load_explicit(&_isWaitingAfterPASEEstablished, memory_order_relaxed);
+}
+
+- (void)setIsWaitingAfterPASEEstablished:(BOOL)isWaitingAfterPASEEstablished
+{
+    BOOL previous = atomic_exchange_explicit(&_isWaitingAfterPASEEstablished, isWaitingAfterPASEEstablished, memory_order_relaxed);
+    if (previous && !isWaitingAfterPASEEstablished) {
+        // The client has either started commissioning (via commissionNodeWithID:)
+        // or moved on; the watchdog no longer needs to fire.  Cancel it on
+        // _delegateQueue, which is where _postPASEWatchdog is owned.
+        dispatch_async(_delegateQueue, ^{
+            [self _cancelPostPASEWatchdog];
+        });
+    }
 }
 
 - (instancetype)initWithParameters:(MTRCommissioningParameters *)parameters
@@ -92,8 +155,14 @@ - (instancetype)initWithParameters:(MTRCommissioningParameters *)parameters
     }
 
     _parameters = [parameters copy];
-    _setupPayload = payload;
-    _commissioningID = commissioningID;
+    // Honor the `copy` semantic on the @property declaration: a caller passing
+    // an NSMutableString must not be able to mutate our stored payload after
+    // construction.  The implicit-cancel path in setupCommissioningSessionWithPayload:
+    // compares the stored payload against the caller's freshly-derived
+    // pairingCode via -isEqualToString:, so any post-construction drift here
+    // would silently corrupt that decision.
+    _setupPayload = [payload copy];
+    _commissioningID = [commissioningID copy];
     _isInternallyCreated = isInternallyCreated;
     _delegate = delegate;
     _delegateQueue = queue;
@@ -104,6 +173,112 @@ - (instancetype)initWithParameters:(MTRCommissioningParameters *)parameters
     return self;
 }
 
+- (void)dealloc
+{
+    // Defensive: ensure the watchdog timer doesn't outlive us.  No other strong
+    // reference can exist (or we wouldn't be deallocating) and the timer's
+    // event_handler retains self for its duration, so it cannot be in flight
+    // here; a synchronous cancel is safe.
+    if (_postPASEWatchdog) {
+        dispatch_source_cancel(_postPASEWatchdog);
+        _postPASEWatchdog = nil;
+    }
+}
+
+// Must be called on _delegateQueue.
+- (BOOL)_armPostPASEWatchdog
+{
+    if (_postPASEWatchdog) {
+        // Already armed; should not happen, but be defensive.
+        return YES;
+    }
+
+    NSTimeInterval intervalSeconds = kMTRPostPASEWatchdogInterval;
+#if DEBUG
+    NSTimeInterval intervalOverride = atomic_load_explicit(&sMTRPostPASEWatchdogIntervalForTesting, memory_order_relaxed);
+    if (intervalOverride > 0) {
+        intervalSeconds = intervalOverride;
+    }
+#endif
+
+    dispatch_source_t timer = dispatch_source_create(DISPATCH_SOURCE_TYPE_TIMER, 0, 0, _delegateQueue);
+    if (!timer) {
+        MTR_LOG_ERROR("%@ failed to create post-PASE watchdog timer", self);
+        return NO;
+    }
+
+    dispatch_source_set_timer(timer,
+        dispatch_time(DISPATCH_TIME_NOW, (int64_t) (intervalSeconds * NSEC_PER_SEC)),
+        DISPATCH_TIME_FOREVER,
+        1 * NSEC_PER_SEC);
+
+    mtr_weakify(self);
+    dispatch_source_set_event_handler(timer, ^{
+        mtr_strongify(self);
+        if (!self) {
+            return;
+        }
+        // Late-fire guard: the timer block may already have been enqueued on
+        // _delegateQueue ahead of a cancel that flipped the waiting flag back
+        // to NO.  If the client has legitimately advanced, do not fire.
+        if (!self.isWaitingAfterPASEEstablished) {
+            return;
+        }
+        [self _firePostPASEWatchdog];
+    });
+
+    dispatch_resume(timer);
+    _postPASEWatchdog = timer;
+    return YES;
+}
+
+// Must be called on _delegateQueue (it mutates _postPASEWatchdog).
+- (void)_cancelPostPASEWatchdog
+{
+    if (_postPASEWatchdog) {
+        dispatch_source_cancel(_postPASEWatchdog);
+        _postPASEWatchdog = nil;
+    }
+}
+
+// Must be called on _delegateQueue (it is the watchdog's event_handler).
+- (void)_firePostPASEWatchdog
+{
+    [self _cancelPostPASEWatchdog];
+
+    MTRDeviceController_Concrete * strongController = _controller;
+    MTR_LOG_ERROR("%@ post-PASE watchdog fired for commissioning %@ -- client never advanced past paseSessionEstablishmentComplete; cancelling commissioning to release controller", self, _commissioningID);
+
+    if (!strongController) {
+        // Controller is gone -- the wedge we wanted to prevent is moot (no
+        // controller state to reclaim), but still drive our own terminal error
+        // so the delegate state settles.
+        [self _dispatchCommissioningError:[MTRError errorForCHIPErrorCode:CHIP_ERROR_TIMEOUT]];
+        return;
+    }
+
+    // If a successor MTRCommissioningOperation has already taken our slot, that
+    // path already cleared our delegate via commissioningDone:, so do not
+    // dispatch a terminal error -- it would tear down the successor's state.
+    if (strongController.currentCommissioning != self) {
+        MTR_LOG("%@ post-PASE watchdog fired but commissioning %@ was already replaced; suppressing spurious timeout", self, _commissioningID);
+        return;
+    }
+
+    // Single-notification contract: we want to deliver exactly one terminal
+    // error -- TIMEOUT, the user-visible "we gave up waiting for you to advance
+    // past PASE" signal -- rather than letting -_cancelCommissioning: emit its
+    // own CANCELLED.  Unlike the client-initiated -stop path (which must leave
+    // isWaitingAfterPASEEstablished YES so -_cancelCommissioning: synthesizes
+    // the completion that frees the controller), here *we* own the terminal
+    // notification, so clear the flag first -- on _delegateQueue, where it is
+    // owned -- so -_cancelCommissioning: observes NO and stays quiet.  The
+    // watchdog timer is already cancelled above.
+    atomic_store_explicit(&_isWaitingAfterPASEEstablished, NO, memory_order_relaxed);
+    [self stop];
+    [self _dispatchCommissioningError:[MTRError errorForCHIPErrorCode:CHIP_ERROR_TIMEOUT]];
+}
+
 static inline void emitMetricForSetupPayload(NSString * payload)
 {
     std::vector<SetupPayload> payloads;
@@ -169,15 +344,52 @@ - (void)startWithController:(MTRDeviceController *)controller
 
 - (BOOL)stop
 {
+    // Tear down the watchdog *timer* but deliberately do NOT clear
+    // isWaitingAfterPASEEstablished here.  -_cancelCommissioning:forNodeID:error:
+    // reads that flag to decide whether it must synthesize the
+    // OnCommissioningComplete(CANCELLED) notification itself: when we are parked
+    // at the post-PASE wait state there is no in-flight PASE/commissioning on
+    // the C++ side, so StopPairing produces no callback and the only thing that
+    // drives the client's completion (and releases the controller's single
+    // commissioning slot) is that synthesized notification.  If we cleared the
+    // flag before calling into the controller, -_cancelCommissioning: would
+    // observe NO, skip the synthesized completion, and the operation would hang
+    // forever -- wedging the controller for all subsequent commissioning.
+    // -_cancelCommissioning: itself clears the flag once it has fired the
+    // notification.
+    //
+    // Cancelling the timer is sufficient to satisfy the watchdog's purpose for
+    // this code path: an explicit stop is exactly the "client advanced/gave up"
+    // signal the watchdog exists to substitute for, so it must not fire after.
+    dispatch_async(_delegateQueue, ^{
+        [self _cancelPostPASEWatchdog];
+    });
+
     MTRDeviceController_Concrete * strongController = _controller;
     if (!strongController) {
         // Nothing to do; controller is gone, so we are stopped no matter what.
         return NO;
     }
-
     return [strongController stopCommissioning:self forCommissioningID:_commissioningID];
 }
 
+// Clears the waiting-after-PASE flag and tears down the watchdog.  Safe to call
+// from any queue: the flag write is atomic, and the dispatch_source cancel is
+// bounced onto _delegateQueue, which owns _postPASEWatchdog.
+//
+// Use this on terminal paths that do NOT route through -_cancelCommissioning:
+// (e.g. -_dispatchCommissioningError: and the terminal-success notification),
+// where there is no downstream consumer of the flag.  Do NOT use it ahead of a
+// -_cancelCommissioning: call that needs to observe the flag as YES so it can
+// synthesize OnCommissioningComplete (see -stop).
+- (void)_clearPostPASEWaiting
+{
+    atomic_store_explicit(&_isWaitingAfterPASEEstablished, NO, memory_order_relaxed);
+    dispatch_async(_delegateQueue, ^{
+        [self _cancelPostPASEWatchdog];
+    });
+}
+
 - (void)_earlyFailCommissioning:(CHIP_ERROR)error
 {
     // This handles failures before we have cleared the way for doing a commissioning at all, and in
@@ -208,6 +420,10 @@ - (void)_dispatchCommissioningError:(NSError *)error withMetrics:(MTRMetrics *)m
 
 - (void)_dispatchCommissioningError:(NSError *)error forCommissioningID:(NSNumber *)commissioningID withMetrics:(MTRMetrics *)metrics
 {
+    // Any terminal error path implies the watchdog (if armed) no longer needs
+    // to fire, so clear the waiting flag and tear it down.
+    [self _clearPostPASEWaiting];
+
     MTRDeviceController_Concrete * strongController = _controller;
 
     MTR_LOG("%@ Device commissioning failed with controller %@ metrics %@", self, strongController, metrics);
@@ -263,7 +479,21 @@ - (void)controller:(MTRDeviceController *)controller commissioningSessionEstabli
     // commissioning ourselves if not.
     if ([strongDelegate respondsToSelector:@selector(commissioning:paseSessionEstablishmentComplete:)]) {
         dispatch_async(_delegateQueue, ^{
-            self.isWaitingAfterPASEEstablished = YES;
+            // Arm the watchdog and set the waiting flag on _delegateQueue, which
+            // is where all watchdog mutations happen.  A client that handles
+            // paseSessionEstablishmentComplete: but then never calls
+            // commissionNodeWithID: or cancelCommissioningForNodeID: would
+            // otherwise permanently wedge the controller's commissioning state.
+            BOOL armed = [self _armPostPASEWatchdog];
+            if (!armed) {
+                // Failing to arm the watchdog means we cannot bound the
+                // post-PASE wait -- the very wedge this logic prevents.  Surface
+                // a no-memory error so the client tears down.
+                MTR_LOG_ERROR("%@ failed to arm post-PASE watchdog; aborting commissioning to avoid an unbounded wait", self);
+                [self _dispatchCommissioningError:[MTRError errorForCHIPErrorCode:CHIP_ERROR_NO_MEMORY]];
+                return;
+            }
+            atomic_store_explicit(&self->_isWaitingAfterPASEEstablished, YES, memory_order_relaxed);
             [strongDelegate commissioning:self paseSessionEstablishmentComplete:error];
         });
 
@@ -303,6 +533,12 @@ - (void)controller:(MTRDeviceController *)controller
         return;
     }
 
+    // Defensive: on the success path the controller should already have flipped
+    // isWaitingAfterPASEEstablished back to NO (from commissionNodeWithID:), but
+    // this is the terminal success notification, so clear and tear down the
+    // watchdog unconditionally rather than depending on that upstream invariant.
+    [self _clearPostPASEWaiting];
+
     id<MTRCommissioningDelegate> strongDelegate = _delegate;
     MTRDeviceController_Concrete * strongController = _controller;
 
@@ -502,3 +738,22 @@ - (void)deviceAttestationCompletedForController:(MTRDeviceController *)controlle
 }
 
 @end
+
+#if DEBUG
+#pragma mark - PostPASEWatchdogTesting
+
+@implementation MTRCommissioningOperation (PostPASEWatchdogTesting)
+
++ (void)setPostPASEWatchdogIntervalForTesting:(NSTimeInterval)interval
+{
+    // Clamp non-finite, negative, or absurdly large values to 0 (== "use the
+    // production interval").  _armPostPASEWatchdog casts (interval *
+    // NSEC_PER_SEC) to int64_t, which is UB (and UBSan-trapping) for +inf, NaN,
+    // or values that overflow int64_t after the multiply; 1e9 seconds is well
+    // past any plausible test interval and leaves ample headroom.
+    NSTimeInterval clamped = (isfinite(interval) && interval > 0 && interval < 1e9) ? interval : 0;
+    atomic_store_explicit(&sMTRPostPASEWatchdogIntervalForTesting, clamped, memory_order_relaxed);
+}
+
+@end
+#endif // DEBUG

src/darwin/Framework/CHIP/MTRCommissioningOperation_Internal.h

@@ -38,14 +38,26 @@ NS_ASSUME_NONNULL_BEGIN
 
 @property (nonatomic, readonly, assign) BOOL isInternallyCreated;
 
+// The commissioning identifier this operation was constructed with (the
+// random "future node ID" we use to track the commissioning flow before the
+// commissionee has been assigned a real node ID).  Exposed so callers like
+// MTRDeviceController_Concrete that need to drive cancellation against this
+// commissioning have something to pass to StopPairing without reaching into
+// private ivars.
+@property (nonatomic, readonly, copy) NSNumber * commissioningID;
+
 // True if the commissioning is waiting to resume after PASE has been
 // established and the delegate chose to be notified about that.
 //
 // This is currently only true if isInternallyCreated, and is readwrite because
 // MTRDeviceController_Concrete helps maintain this state.
 //
-// This property should generally be written on client queues only, not on the
-// Matter queue.
+// The value is backed by a C11 atomic so reads and writes are safe from any
+// queue.  Clearing the flag (a YES->NO transition) tears down the paired
+// post-PASE watchdog timer on the operation's delegate queue; the watchdog
+// event_handler also re-checks this flag as a late-fire guard so it does not
+// route a spurious timeout when the client has legitimately advanced but a
+// timer block was already enqueued ahead of the cancel.
 @property (nonatomic, readwrite, assign) BOOL isWaitingAfterPASEEstablished;
 
 @end