Fix issue with sync intervals (#884)

mlw · web-flow · commit 8be9e168d991 · 2026-03-30T11:57:49.000-04:00
* For sync v1 clients, fixes issue where sync intervals are not properly
respected.
* Moves to dynamic `pushNotifications` initialization.
* Fixes `santactl status` display issues
* Fixes a very rare sync service startup race where the initial sync
reschedule could be set for 4 hours into the future.
diff --git a/Source/common/SNTConfigurator.h b/Source/common/SNTConfigurator.h
@@ -768,7 +768,7 @@ extern NSString* _Nonnull const kEnableMenuItemUserOverride;
 ///
 ///  Set the full sync interval as received from a sync server.
 ///
-- (void)setFullSyncInterval:(NSUInteger)interval;
+- (void)setSyncServerFullSyncInterval:(NSUInteger)interval;
 
 ///
 ///  Full sync interval in seconds while listening for push notifications.
@@ -779,7 +779,7 @@ extern NSString* _Nonnull const kEnableMenuItemUserOverride;
 ///
 ///  Set the push notifications full sync interval as received from a sync server.
 ///
-- (void)setPushNotificationsFullSyncInterval:(NSUInteger)interval;
+- (void)setSyncServerPushNotificationsFullSyncInterval:(NSUInteger)interval;
 
 ///
 ///  Enable statistics uploading to polaris.northpole.security.
diff --git a/Source/common/SNTConfigurator.mm b/Source/common/SNTConfigurator.mm
@@ -1348,7 +1348,7 @@ - (NSUInteger)fullSyncInterval {
   return kDefaultFullSyncInterval;
 }
 
-- (void)setFullSyncInterval:(NSUInteger)interval {
+- (void)setSyncServerFullSyncInterval:(NSUInteger)interval {
   [self updateSyncStateForKey:kFullSyncInterval value:@(interval)];
 }
 
@@ -1360,8 +1360,10 @@ - (NSUInteger)pushNotificationsFullSyncInterval {
   return kDefaultPushNotificationsFullSyncInterval;
 }
 
-- (void)setPushNotificationsFullSyncInterval:(NSUInteger)interval {
-  [self updateSyncStateForKey:kFCMFullSyncInterval value:@(interval)];
+- (void)setSyncServerPushNotificationsFullSyncInterval:(NSUInteger)interval {
+  // A value of 0 means "not applicable" (e.g. sync v1). Clear the key so
+  // the getter falls back to the default rather than storing a sentinel.
+  [self updateSyncStateForKey:kFCMFullSyncInterval value:interval ? @(interval) : nil];
 }
 
 - (NSString*)machineOwner {
diff --git a/Source/santactl/Commands/SNTCommandStatus.mm b/Source/santactl/Commands/SNTCommandStatus.mm
@@ -386,7 +386,7 @@ - (void)runWithArguments:(NSArray*)arguments {
         @"full_sync_interval_seconds" : @(fullSyncInterval),
       } mutableCopy];
 
-      if (configurator.fcmEnabled || configurator.enablePushNotifications) {
+      if (configurator.fcmEnabled || (isSyncV2Enabled && configurator.enablePushNotifications)) {
         stats[@"sync"][@"push_notifications_full_sync_interval_seconds"] =
             @(pushNotificationsFullSyncInterval);
       }
@@ -511,10 +511,10 @@ - (void)runWithArguments:(NSArray*)arguments {
       printf("  %-40s | %s\n", "Last Successful Full Sync", [fullSyncLastSuccessStr UTF8String]);
       printf("  %-40s | %s\n", "Last Successful Rule Sync", [ruleSyncLastSuccessStr UTF8String]);
 
-      // If push notifications are enabled, show the push notifications full
-      // sync interval since it's the active configuration.
+      // Show the push notifications full sync interval for FCM clients or
+      // sync v2 clients where push has not been explicitly disabled.
       NSString* fullSyncIntervalStr = FormatInterval(fullSyncInterval);
-      if (configurator.fcmEnabled || configurator.enablePushNotifications) {
+      if (configurator.fcmEnabled || (isSyncV2Enabled && configurator.enablePushNotifications)) {
         fullSyncIntervalStr =
             [NSString stringWithFormat:@"%@ (with Push Notifications)",
                                        FormatInterval(pushNotificationsFullSyncInterval)];
diff --git a/Source/santad/SNTDaemonControlController.mm b/Source/santad/SNTDaemonControlController.mm
@@ -591,11 +591,11 @@ - (void)updateSyncSettings:(SNTConfigBundle*)result reply:(void (^)(void))reply
   }];
 
   [result fullSyncInterval:^(NSUInteger val) {
-    [configurator setFullSyncInterval:val];
+    [configurator setSyncServerFullSyncInterval:val];
   }];
 
   [result pushNotificationsFullSyncInterval:^(NSUInteger val) {
-    [configurator setPushNotificationsFullSyncInterval:val];
+    [configurator setSyncServerPushNotificationsFullSyncInterval:val];
   }];
 
   reply();
diff --git a/Source/santasyncservice/BUILD b/Source/santasyncservice/BUILD
@@ -424,7 +424,6 @@ objc_library(
         "//Source/common:MOLXPCConnection",
         "//Source/common:SNTDropRootPrivs",
         "//Source/common:SNTExportConfiguration",
-        "//Source/common:SNTKVOManager",
         "//Source/common:SNTLogging",
         "//Source/common:SNTStrengthify",
         "//Source/common:SNTXPCControlInterface",
diff --git a/Source/santasyncservice/SNTPushClientNATS.h b/Source/santasyncservice/SNTPushClientNATS.h
@@ -16,5 +16,6 @@
 
 @interface SNTPushClientNATS : NSObject <SNTPushNotificationsClientDelegate>
 - (instancetype)initWithSyncDelegate:(id<SNTPushNotificationsSyncDelegate>)syncDelegate;
+- (void)disconnectWithCompletion:(void (^)(void))completion;
 @property(nonatomic, readonly, copy) NSString* pushServer;
 @end
diff --git a/Source/santasyncservice/SNTPushClientNATS.mm b/Source/santasyncservice/SNTPushClientNATS.mm
@@ -982,6 +982,13 @@ - (void)handlePreflightSyncState:(SNTSyncState*)syncState {
 
     // Now attempt to connect
     [self connect];
+
+    // Only update the sync interval when we have valid push configuration.
+    // Without this guard, sync v1 clients (with no push infrastructure) would
+    // have their interval overwritten to the minimum (60s).
+    if (syncState.pushNotificationsFullSyncInterval) {
+      self.fullSyncInterval = syncState.pushNotificationsFullSyncInterval.unsignedIntegerValue;
+    }
   } else {
     NSMutableArray* missing = [NSMutableArray array];
     if (!syncState.pushServer) [missing addObject:@"server"];
@@ -992,15 +999,13 @@ - (void)handlePreflightSyncState:(SNTSyncState*)syncState {
          [missing componentsJoinedByString:@", "]);
   }
 
+  // HMAC key is independent of connection credentials — update or clear
+  // regardless of whether the full push config was present.
   if (syncState.pushHMACKey) {
     self.hmacKey = syncState.pushHMACKey;
   } else {
     LOGW(@"NATS: No push HMAC key received from preflight");
-  }
-
-  // Update sync interval to avoid polling Workshop.
-  if (syncState.pushNotificationsFullSyncInterval) {
-    self.fullSyncInterval = syncState.pushNotificationsFullSyncInterval.unsignedIntegerValue;
+    self.hmacKey = nil;
   }
 }
 
diff --git a/Source/santasyncservice/SNTPushClientNATSTest.mm b/Source/santasyncservice/SNTPushClientNATSTest.mm
@@ -219,8 +219,12 @@ - (void)testHandlePreflightSyncStateUpdatesInterval {
   self.client = [[SNTPushClientNATS alloc] initWithSyncDelegate:self.mockSyncDelegate];
   NSUInteger originalInterval = self.client.fullSyncInterval;
 
-  // When: Preflight sync state with new interval is handled
+  // When: Preflight sync state with new interval and valid push config is handled
   SNTSyncState* syncState = [[SNTSyncState alloc] init];
+  syncState.pushServer = @"workshop";
+  syncState.pushNKey = @"test-nkey";
+  syncState.pushJWT = @"test-jwt";
+  syncState.pushDeviceID = @"test-device-id";
   syncState.pushNotificationsFullSyncInterval = @(7200);  // 2 hours
 
   [self.client handlePreflightSyncState:syncState];
@@ -230,6 +234,21 @@ - (void)testHandlePreflightSyncStateUpdatesInterval {
   XCTAssertNotEqual(self.client.fullSyncInterval, originalInterval);
 }
 
+- (void)testHandlePreflightSyncStateDoesNotUpdateIntervalWithoutPushConfig {
+  // Given: Client is initialized
+  self.client = [[SNTPushClientNATS alloc] initWithSyncDelegate:self.mockSyncDelegate];
+  NSUInteger originalInterval = self.client.fullSyncInterval;
+
+  // When: Preflight sync state has interval but no push config fields
+  SNTSyncState* syncState = [[SNTSyncState alloc] init];
+  syncState.pushNotificationsFullSyncInterval = @(7200);
+
+  [self.client handlePreflightSyncState:syncState];
+
+  // Then: Interval should NOT be updated (no push config)
+  XCTAssertEqual(self.client.fullSyncInterval, originalInterval);
+}
+
 #pragma mark - Full Sync Interval Tests
 
 - (void)testFullSyncIntervalDefaultValue {
diff --git a/Source/santasyncservice/SNTSyncManager.mm b/Source/santasyncservice/SNTSyncManager.mm
@@ -84,17 +84,19 @@ - (instancetype)initWithDaemonConnection:(MOLXPCConnection*)daemonConn {
     if (config.fcmEnabled) {
       LOGD(@"Using FCM push notifications");
       _pushNotifications = [[SNTPushClientFCM alloc] initWithSyncDelegate:self];
-    } else if (config.enablePushNotifications) {
-      // Unless explicitly disabled, initialize the NATS push client. The client
-      // will wait for configuration during a valid V2 preflight.
-      LOGD(@"Using NATS push notifications");
-      _pushNotifications = [[SNTPushClientNATS alloc] initWithSyncDelegate:self];
     }
+    // NATS push client is created dynamically in preflightWithSyncState: after
+    // the server provides a validated token chain (isSyncV2) and push config.
 
     _fullSyncTimer = [self createSyncTimerWithBlock:^{
-      [self rescheduleTimerQueue:self.fullSyncTimer
-                  secondsFromNow:_pushNotifications ? _pushNotifications.fullSyncInterval
-                                                    : kDefaultFullSyncInterval];
+      // Reschedule as a fallback before starting the sync. On success,
+      // preflightWithSyncState: will override with the server-informed interval.
+      // On failure, the failed-preflight path corrects it as well. This is just
+      // a safety net for the case where the sync fails before reaching any
+      // rescheduling logic.
+      NSUInteger interval = self.pushNotifications ? self.pushNotifications.fullSyncInterval
+                                                   : kDefaultFullSyncInterval;
+      [self rescheduleTimerQueue:self.fullSyncTimer secondsFromNow:interval];
       [self syncType:SNTSyncTypeNormal withReply:NULL];
     }];
     _ruleSyncTimer = [self createSyncTimerWithBlock:^{
@@ -523,47 +525,76 @@ - (SNTSyncStatusType)preflightWithSyncState:(SNTSyncState*)syncState {
 
     self.eventBatchSize = syncState.eventBatchSize;
 
-    // Start listening for push notifications with a full sync every
-    // pushNotificationsFullSyncInterval.
+    // Dynamic NATS lifecycle: create when server validates sync v2,
+    // tear down when sync v2 is no longer valid.
+    if (syncState.isSyncV2 && !self.pushNotifications) {
+      // Check kill switch — respect explicit admin disable
+      if ([[SNTConfigurator configurator] enablePushNotifications]) {
+        LOGI(@"Creating NATS push client after successful sync v2 preflight");
+        self.pushNotifications = [[SNTPushClientNATS alloc] initWithSyncDelegate:self];
+      }
+    } else if (!syncState.isSyncV2 && self.pushNotifications &&
+               [self.pushNotifications isKindOfClass:[SNTPushClientNATS class]]) {
+      // Token chain no longer valid — tear down NATS client.
+      // The NATS C library holds unretained pointers to the client via __bridge
+      // callbacks, so we must disconnect (which destroys the C-level connection)
+      // before the object is deallocated. disconnectWithCompletion: dispatches
+      // async onto connectionQueue and the block's implicit capture of self (the
+      // NATS client) keeps it alive until cleanup completes.
+      LOGI(@"Sync v2 no longer active, tearing down NATS push client");
+      [(SNTPushClientNATS*)self.pushNotifications disconnectWithCompletion:nil];
+      self.pushNotifications = nil;
+    }
+
+    // pushNotificationsFullSyncInterval is only meaningful when push notifications
+    // are in use. Clear it otherwise so the postflight persists 0 to the daemon,
+    // overriding any stale default (14400).
+    if (!self.pushNotifications) {
+      syncState.pushNotificationsFullSyncInterval = @(0);
+    }
+
+    // Hand off push credentials to the push client (if present).
     if (self.pushNotifications) {
       NSUInteger oldInterval = self.pushNotifications.fullSyncInterval;
       [self.pushNotifications handlePreflightSyncState:syncState];
 
-      // Clear all push credentials from syncState after handoff to push client
-      // These are no longer needed and should not be accessible to other sync stages
+      // Clear all push credentials from syncState after handoff to push client.
+      // These are no longer needed and should not be accessible to other sync stages.
       syncState.pushNKey = nil;
       syncState.pushJWT = nil;
       syncState.pushIssuerJWT = nil;
       syncState.pushHMACKey = nil;
 
-      // If push interval changed, mark log the difference.
       if (oldInterval != self.pushNotifications.fullSyncInterval) {
         LOGD(
             @"Push notification sync interval changed from %lu to %lu seconds. Rescheduling timer.",
             oldInterval, self.pushNotifications.fullSyncInterval);
       }
+    }
 
-      // Always reschedule
+    // Use the push notification interval when the server provided one and we
+    // have an active push client. syncState.pushNotificationsFullSyncInterval
+    // is nil when the server does not set push_notification_full_sync_interval_seconds
+    // (e.g. sync v1). In that case, fall back to the server's regular full_sync_interval.
+    if (self.pushNotifications && syncState.pushNotificationsFullSyncInterval) {
       [self rescheduleTimerQueue:self.fullSyncTimer
                   secondsFromNow:self.pushNotifications.fullSyncInterval];
     } else {
       NSUInteger interval = syncState.fullSyncInterval
                                 ? syncState.fullSyncInterval.unsignedIntegerValue
                                 : kDefaultFullSyncInterval;
-      LOGD(@"Push notifications are not enabled. Sync every %lu min.", interval / 60);
-
-      // Always reschedule
+      LOGD(@"Push notifications not configured by server. Sync every %lu min.", interval / 60);
       [self rescheduleTimerQueue:self.fullSyncTimer secondsFromNow:interval];
     }
 
     if (syncState.preflightOnly) return SNTSyncStatusTypeSuccess;
     return [self eventUploadWithSyncState:syncState];
-  } else if (_pushNotifications) {
+  } else if (self.pushNotifications) {
     // If preflight failed and push notifications are enabled, force a reschedule for
     // the smaller of the default sync interval (default 10 minutes) and whatever the
     // last push full sync interval was set to (default 4 hours).
     // If push notifications are not enabled, the default sync interval was already set (10m).
-    auto interval = std::min(_pushNotifications.fullSyncInterval, kDefaultFullSyncInterval);
+    auto interval = std::min(self.pushNotifications.fullSyncInterval, kDefaultFullSyncInterval);
     [self rescheduleTimerQueue:self.fullSyncTimer secondsFromNow:interval];
   }
 
@@ -621,6 +652,10 @@ - (dispatch_source_t)createSyncTimerWithBlock:(void (^)(void))block {
       block();
     }
   });
+  // Arm with DISPATCH_TIME_FOREVER so the timer does not fire until explicitly
+  // scheduled via rescheduleTimerQueue:secondsFromNow: (e.g. syncSecondsFromNow:15).
+  // Without this, the default fire time races with the initial schedule call.
+  dispatch_source_set_timer(timerQueue, DISPATCH_TIME_FOREVER, DISPATCH_TIME_FOREVER, 0);
   dispatch_resume(timerQueue);
   return timerQueue;
 }
diff --git a/Source/santasyncservice/SNTSyncManagerNATSTest.mm b/Source/santasyncservice/SNTSyncManagerNATSTest.mm
@@ -49,47 +49,40 @@ - (void)tearDown {
   [super tearDown];
 }
 
-- (void)testSyncManagerInitializesNATSClientWhenEnabled {
-  // Given: FCM is disabled, NATS is enabled (default), and syncBaseURL is pinned
+- (void)testSyncManagerDoesNotCreateNATSClientAtStartup {
+  // NATS client should NOT be created at startup, even when enablePushNotifications is YES.
+  // It is created dynamically after preflight token chain validation.
   OCMStub([self.mockConfigurator fcmEnabled]).andReturn(NO);
   OCMStub([self.mockConfigurator enablePushNotifications]).andReturn(YES);
   OCMStub([self.mockConfigurator syncBaseURL])
       .andReturn([NSURL URLWithString:@"https://example.workshop.cloud"]);
 
-  // When: Sync manager is initialized
   self.syncManager = [[SNTSyncManager alloc] initWithDaemonConnection:self.mockDaemonConn];
 
-  // Then: Push notifications should be NATS client
-  XCTAssertNotNil(self.syncManager.pushNotifications);
-  XCTAssertTrue([self.syncManager.pushNotifications isKindOfClass:[SNTPushClientNATS class]]);
+  XCTAssertNil(self.syncManager.pushNotifications, @"NATS client should not be created at startup");
 }
 
-- (void)testSyncManagerFCMTakesPrecedenceOverNATS {
-  // Given: Both FCM and NATS are enabled
+- (void)testSyncManagerFCMStillCreatedAtStartup {
+  // FCM is a legacy client and should still be created at startup.
   OCMStub([self.mockConfigurator fcmEnabled]).andReturn(YES);
-  OCMStub([self.mockConfigurator enablePushNotifications]).andReturn(YES);
   OCMStub([self.mockConfigurator syncBaseURL])
       .andReturn([NSURL URLWithString:@"https://example.workshop.cloud"]);
 
-  // When: Sync manager is initialized
   self.syncManager = [[SNTSyncManager alloc] initWithDaemonConnection:self.mockDaemonConn];
 
-  // Then: Push notifications should be FCM client (FCM takes precedence)
   XCTAssertNotNil(self.syncManager.pushNotifications);
   XCTAssertTrue([self.syncManager.pushNotifications isKindOfClass:[SNTPushClientFCM class]]);
 }
 
-- (void)testSyncManagerNoPushClientWhenAllDisabled {
-  // Given: All push notification systems are disabled
+- (void)testSyncManagerNoPushClientWhenFCMDisabled {
+  // No push client at startup when FCM is disabled (NATS is dynamic now).
   OCMStub([self.mockConfigurator fcmEnabled]).andReturn(NO);
   OCMStub([self.mockConfigurator enablePushNotifications]).andReturn(NO);
   OCMStub([self.mockConfigurator syncBaseURL])
       .andReturn([NSURL URLWithString:@"https://example.workshop.cloud"]);
 
-  // When: Sync manager is initialized
   self.syncManager = [[SNTSyncManager alloc] initWithDaemonConnection:self.mockDaemonConn];
 
-  // Then: Push notifications should be nil
   XCTAssertNil(self.syncManager.pushNotifications);
 }
 
diff --git a/Source/santasyncservice/SNTSyncManagerTest.mm b/Source/santasyncservice/SNTSyncManagerTest.mm
@@ -138,6 +138,33 @@ - (void)testCreateSyncTimerWithBlock {
   dispatch_source_cancel(timer);
 }
 
+- (void)testCreateSyncTimerDoesNotFireImmediately {
+  // Verify that a newly created timer does not fire until explicitly scheduled.
+  // This prevents a race with the initial syncSecondsFromNow:15 call.
+  id mockConnection = OCMClassMock([MOLXPCConnection class]);
+  SNTSyncManager* syncManager = [[SNTSyncManager alloc] initWithDaemonConnection:mockConnection];
+
+  __block BOOL blockFired = NO;
+  dispatch_source_t timer = [syncManager createSyncTimerWithBlock:^{
+    blockFired = YES;
+  }];
+
+  XCTAssertNotNil(timer, @"Timer should be created");
+
+  // Wait 100ms — more than enough for an unguarded timer to fire
+  dispatch_semaphore_t sema = dispatch_semaphore_create(0);
+  dispatch_after(dispatch_time(DISPATCH_TIME_NOW, 100 * NSEC_PER_MSEC),
+                 dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
+                   dispatch_semaphore_signal(sema);
+                 });
+  dispatch_semaphore_wait(sema, DISPATCH_TIME_FOREVER);
+
+  XCTAssertFalse(blockFired, @"Timer block should not fire until explicitly scheduled");
+
+  // Clean up
+  dispatch_source_cancel(timer);
+}
+
 - (void)testTimerReschedulingAfterIntervalChange {
   // Test that timers are properly rescheduled when intervals change
   // This verifies the integration between interval updates and timer rescheduling
diff --git a/Source/santasyncservice/SNTSyncPreflight.mm b/Source/santasyncservice/SNTSyncPreflight.mm
@@ -186,10 +186,14 @@ BOOL Preflight(SNTSyncPreflight* self, google::protobuf::Arena* arena,
     self.syncState.eventBatchSize = resp.batch_size();
   }
 
-  // Don't let these go too low
+  // Only set pushNotificationsFullSyncInterval when the server actually provides a value.
+  // If neither field is set (e.g. sync v1 servers), leave the property nil so that the
+  // push client doesn't adopt a bogus minimum interval.
   uint64_t value = resp.push_notification_full_sync_interval_seconds()
                        ?: resp.deprecated_fcm_full_sync_interval_seconds();
-  self.syncState.pushNotificationsFullSyncInterval = @(MAX(value, kMinimumFullSyncInterval));
+  if (value > 0) {
+    self.syncState.pushNotificationsFullSyncInterval = @(MAX(value, kMinimumFullSyncInterval));
+  }
 
   value = resp.push_notification_global_rule_sync_deadline_seconds()
               ?: resp.deprecated_fcm_global_rule_sync_deadline_seconds();
diff --git a/Source/santasyncservice/SNTSyncService.mm b/Source/santasyncservice/SNTSyncService.mm
diff --git a/Source/santasyncservice/SNTSyncTest.mm b/Source/santasyncservice/SNTSyncTest.mm

Original file line number	Diff line number	Diff line change
`@@ -1348,7 +1348,7 @@ - (NSUInteger)fullSyncInterval {`
`1348`	`1348`	`return kDefaultFullSyncInterval;`
`1349`	`1349`	`}`
`1350`	`1350`
`1351`		`-- (void)setFullSyncInterval:(NSUInteger)interval {`
	`1351`	`+- (void)setSyncServerFullSyncInterval:(NSUInteger)interval {`
`1352`	`1352`	`[self updateSyncStateForKey:kFullSyncInterval value:@(interval)];`
`1353`	`1353`	`}`
`1354`	`1354`
`@@ -1360,8 +1360,10 @@ - (NSUInteger)pushNotificationsFullSyncInterval {`
`1360`	`1360`	`return kDefaultPushNotificationsFullSyncInterval;`
`1361`	`1361`	`}`
`1362`	`1362`
`1363`		`-- (void)setPushNotificationsFullSyncInterval:(NSUInteger)interval {`
`1364`		`- [self updateSyncStateForKey:kFCMFullSyncInterval value:@(interval)];`
	`1363`	`+- (void)setSyncServerPushNotificationsFullSyncInterval:(NSUInteger)interval {`
	`1364`	`+ // A value of 0 means "not applicable" (e.g. sync v1). Clear the key so`
	`1365`	`+ // the getter falls back to the default rather than storing a sentinel.`
	`1366`	`+ [self updateSyncStateForKey:kFCMFullSyncInterval value:interval ? @(interval) : nil];`
`1365`	`1367`	`}`
`1366`	`1368`
`1367`	`1369`	`- (NSString*)machineOwner {`