sync: clear sync state on clean sync#934
Closed
sharvilshah wants to merge 6 commits into
Closed
Conversation
github-actions
Bot
added
configurator
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a clearSyncStateBeforeApply flag to SNTConfigBundle, implements atomic bundle application and main-thread clearing of in-memory and on-disk sync state, updates XPC docs and daemon wiring to use the atomic path for clean syncs, and adds tests and BUILD updates covering these behaviors. ChangesSync State Cleanup Refactoring
Sequence DiagramsequenceDiagram
participant Client
participant Daemon
participant Configurator
participant Disk
participant CELCache
Client->>Daemon: request sync (preflight/postflight)
Daemon->>Configurator: atomicallyApplyBundle(bundle) if bundle.clearSyncStateBeforeApply==YES
Configurator->>Configurator: in-memory swap of syncState
Configurator->>Disk: single write of sync-state.plist
Configurator->>Daemon: apply complete
Daemon->>CELCache: flush caches if CEL fallback rules changed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/santasyncservice/SNTSyncManager.mm (1)
332-353:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRelease
syncLimiteron the daemon-timeout early return.At Line 347, the timeout path returns before the limiter is released at Line 353. This can leak a permit and eventually block future syncs with
SNTSyncStatusTypeTooManySyncsInProgress.🔧 Proposed fix
if (dispatch_semaphore_wait(sema, dispatch_time(DISPATCH_TIME_NOW, 2 * NSEC_PER_SEC))) { SLOGE(@"Timeout waiting for daemon"); if (reply) reply(SNTSyncStatusTypeDaemonTimeout); + dispatch_semaphore_signal(self.syncLimiter); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/santasyncservice/SNTSyncManager.mm` around lines 332 - 353, The daemon-timeout path in the dispatch_async block releases early without signaling self.syncLimiter, leaking a permit; before returning on the timeout (the branch after dispatch_semaphore_wait(sema,...)), call dispatch_semaphore_signal(self.syncLimiter) (and then invoke reply(SNTSyncStatusTypeDaemonTimeout)) so the semaphore is always released when aborting, keeping syncLimiter consistent for future calls (affecting the block that creates sema and calls [[self.daemonConn remoteObjectProxy] updateSyncSettings:...] and the surrounding reply/preflightWithKeepOldSettings logic).
🧹 Nitpick comments (1)
Source/santasyncservice/SNTSyncTest.mm (1)
1168-1218: ⚡ Quick winStrengthen routing tests with negative expectations.
At Line 1178, Line 1191, Line 1204, and Line 1217 you only verify the expected selector was called. These tests can still pass if both
replaceSyncSettings:andupdateSyncSettings:are invoked in the same run. AddOCMRejectfor the non-expected path in each test.♻️ Suggested test hardening
- (void)testPostflightCleanSyncRoutesToReplaceSyncSettings { [self setupDefaultDaemonConnResponses]; self.syncState.syncType = SNTSyncTypeClean; self.syncState.keepOldSettings = NO; SNTSyncPostflight* sut = [[SNTSyncPostflight alloc] initWithState:self.syncState]; [self stubRequestBody:nil response:nil error:nil validateBlock:nil]; + OCMReject([self.daemonConnRop updateSyncSettings:OCMOCK_ANY reply:OCMOCK_ANY]); XCTAssertTrue([sut sync]); OCMVerify([self.daemonConnRop replaceSyncSettings:OCMOCK_ANY reply:OCMOCK_ANY]); } - (void)testPostflightCleanAllSyncRoutesToReplaceSyncSettings { [self setupDefaultDaemonConnResponses]; self.syncState.syncType = SNTSyncTypeCleanAll; self.syncState.keepOldSettings = NO; SNTSyncPostflight* sut = [[SNTSyncPostflight alloc] initWithState:self.syncState]; [self stubRequestBody:nil response:nil error:nil validateBlock:nil]; + OCMReject([self.daemonConnRop updateSyncSettings:OCMOCK_ANY reply:OCMOCK_ANY]); XCTAssertTrue([sut sync]); OCMVerify([self.daemonConnRop replaceSyncSettings:OCMOCK_ANY reply:OCMOCK_ANY]); } - (void)testPostflightCleanSyncWithKeepOldSettingsRoutesToUpdateSyncSettings { [self setupDefaultDaemonConnResponses]; self.syncState.syncType = SNTSyncTypeClean; self.syncState.keepOldSettings = YES; SNTSyncPostflight* sut = [[SNTSyncPostflight alloc] initWithState:self.syncState]; [self stubRequestBody:nil response:nil error:nil validateBlock:nil]; + OCMReject([self.daemonConnRop replaceSyncSettings:OCMOCK_ANY reply:OCMOCK_ANY]); XCTAssertTrue([sut sync]); OCMVerify([self.daemonConnRop updateSyncSettings:OCMOCK_ANY reply:OCMOCK_ANY]); } - (void)testPostflightCleanAllSyncWithKeepOldSettingsRoutesToUpdateSyncSettings { [self setupDefaultDaemonConnResponses]; self.syncState.syncType = SNTSyncTypeCleanAll; self.syncState.keepOldSettings = YES; SNTSyncPostflight* sut = [[SNTSyncPostflight alloc] initWithState:self.syncState]; [self stubRequestBody:nil response:nil error:nil validateBlock:nil]; + OCMReject([self.daemonConnRop replaceSyncSettings:OCMOCK_ANY reply:OCMOCK_ANY]); XCTAssertTrue([sut sync]); OCMVerify([self.daemonConnRop updateSyncSettings:OCMOCK_ANY reply:OCMOCK_ANY]); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/santasyncservice/SNTSyncTest.mm` around lines 1168 - 1218, For each test (testPostflightCleanSyncRoutesToReplaceSyncSettings, testPostflightCleanAllSyncRoutesToReplaceSyncSettings, testPostflightCleanSyncWithKeepOldSettingsRoutesToUpdateSyncSettings, testPostflightCleanAllSyncWithKeepOldSettingsRoutesToUpdateSyncSettings) add a negative expectation on self.daemonConnRop for the non-expected selector (use OCMReject([self.daemonConnRop replaceSyncSettings:OCMOCK_ANY reply:OCMOCK_ANY]) in the tests that expect updateSyncSettings, and OCMReject([self.daemonConnRop updateSyncSettings:OCMOCK_ANY reply:OCMOCK_ANY]) in the tests that expect replaceSyncSettings) before calling [sut sync] so the test fails if both paths are invoked. Ensure the OCMReject calls are placed after setupDefaultDaemonConnResponses/stubs and before the XCTAssertTrue([sut sync]) invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/common/SNTConfigurator.mm`:
- Around line 2156-2160: The atomic-replace path is writing kFullSyncLastSuccess
without preserving the coupled kRuleSyncLastSuccess and can clear the rule-sync
timestamp; when handling PostflightConfigBundle/fullSyncLastSuccess (see the
bundle fullSyncLastSuccess:block and the
updateSyncStateForKey:kFullSyncLastSuccess call) ensure you also update
kRuleSyncLastSuccess to the same NSDate (use the existing
updateSyncStateForKey:kRuleSyncLastSuccess value:val writeToDisk:NO pattern) so
the full-sync → rule-sync coupling is preserved (or, alternately, change the
replace path to merge existing rule timestamp rather than overwrite it).
---
Outside diff comments:
In `@Source/santasyncservice/SNTSyncManager.mm`:
- Around line 332-353: The daemon-timeout path in the dispatch_async block
releases early without signaling self.syncLimiter, leaking a permit; before
returning on the timeout (the branch after dispatch_semaphore_wait(sema,...)),
call dispatch_semaphore_signal(self.syncLimiter) (and then invoke
reply(SNTSyncStatusTypeDaemonTimeout)) so the semaphore is always released when
aborting, keeping syncLimiter consistent for future calls (affecting the block
that creates sema and calls [[self.daemonConn remoteObjectProxy]
updateSyncSettings:...] and the surrounding reply/preflightWithKeepOldSettings
logic).
---
Nitpick comments:
In `@Source/santasyncservice/SNTSyncTest.mm`:
- Around line 1168-1218: For each test
(testPostflightCleanSyncRoutesToReplaceSyncSettings,
testPostflightCleanAllSyncRoutesToReplaceSyncSettings,
testPostflightCleanSyncWithKeepOldSettingsRoutesToUpdateSyncSettings,
testPostflightCleanAllSyncWithKeepOldSettingsRoutesToUpdateSyncSettings) add a
negative expectation on self.daemonConnRop for the non-expected selector (use
OCMReject([self.daemonConnRop replaceSyncSettings:OCMOCK_ANY reply:OCMOCK_ANY])
in the tests that expect updateSyncSettings, and OCMReject([self.daemonConnRop
updateSyncSettings:OCMOCK_ANY reply:OCMOCK_ANY]) in the tests that expect
replaceSyncSettings) before calling [sut sync] so the test fails if both paths
are invoked. Ensure the OCMReject calls are placed after
setupDefaultDaemonConnResponses/stubs and before the XCTAssertTrue([sut sync])
invocation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e155e47-bbef-4782-ac51-f1286e11cf81
📒 Files selected for processing (17)
Source/common/BUILDSource/common/SNTConfigurator.hSource/common/SNTConfigurator.mmSource/common/SNTConfiguratorTest.mmSource/common/SNTXPCControlInterface.hSource/common/SNTXPCSyncServiceInterface.hSource/gui/SNTAboutWindowView.swiftSource/gui/SNTStatusItemManager.mmSource/santactl/Commands/SNTCommandSync.mmSource/santad/SNTDaemonControlController.mmSource/santasyncservice/SNTSyncConfigBundleTest.mmSource/santasyncservice/SNTSyncManager.hSource/santasyncservice/SNTSyncManager.mmSource/santasyncservice/SNTSyncPostflight.mmSource/santasyncservice/SNTSyncService.mmSource/santasyncservice/SNTSyncState.hSource/santasyncservice/SNTSyncTest.mm
tburgin
reviewed
May 5, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/santad/SNTDaemonControlController.mm`:
- Around line 476-487: The current syncType handler clears persisted sync state
whenever SNTSyncTypeNormal is observed, but SNTSyncTypeNormal can be emitted for
ordinary sync starts; modify the logic around the [result syncType:^(SNTSyncType
val) { ... }] block so that clearSyncState is only called when this Normal
update is coming from a postflight bundle. Concretely, gate the call to
[configurator clearSyncState] by also checking the postflight signal already
present on the bundle (use the bundle/result's fullSyncLastSuccess value): only
call clearSyncState when val == SNTSyncTypeNormal AND the bundle's
fullSyncLastSuccess flag is true (the postflight-only indicator used by
PostflightConfigBundle), leaving other SNTSyncTypeNormal events untouched.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ff2e047-c6e0-4e57-a229-da44a5594ab2
📒 Files selected for processing (9)
Source/common/SNTConfigurator.hSource/common/SNTConfigurator.mmSource/common/SNTConfiguratorTest.mmSource/common/SNTXPCControlInterface.hSource/santactl/Commands/SNTCommandSync.mmSource/santad/SNTDaemonControlController.mmSource/santasyncservice/SNTSyncConfigBundle.mmSource/santasyncservice/SNTSyncConfigBundleTest.mmSource/santasyncservice/SNTSyncManager.h
✅ Files skipped from review due to trivial changes (1)
- Source/santactl/Commands/SNTCommandSync.mm
mlw
previously requested changes
May 6, 2026
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Source/santasyncservice/SNTSyncConfigBundle.mm (1)
72-76: ⚡ Quick winUpdate this comment to describe the new signal.
The daemon no longer takes the atomic-reset path because
syncType == @(Normal); it now keys offclearSyncStateBeforeApply. Leaving the old explanation here makes this flow easy to misread during the next change.✏️ Suggested comment update
- // Clean syncs reset SyncTypeRequired to Normal. The daemon recognizes - // this exact transition (`syncType == @(Normal)`) as the signal that - // this is a clean-sync postflight bundle and clears persisted sync - // state before applying. + // Clean syncs reset SyncTypeRequired to Normal in postflight. + // The daemon's atomic-replace path is driven by + // clearSyncStateBeforeApply; syncType here only resets the persisted + // SyncTypeRequired value back to Normal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/santasyncservice/SNTSyncConfigBundle.mm` around lines 72 - 76, Update the explanatory comment above the bundle.syncType assignment (the block that mentions SyncTypeRequired/Normal) to reflect the new behavior: the daemon no longer uses the atomic-reset trigger of syncType == @(SNTSyncTypeNormal); it now checks the clearSyncStateBeforeApply flag to decide to clear persisted sync state before applying a clean-sync bundle. Mention that bundle.syncType is still set to Normal for backwards compatibility but the actual reset signal is clearSyncStateBeforeApply, and remove or correct the legacy wording that implies the daemon keys off syncType alone.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/santad/SNTDaemonControlController.mm`:
- Around line 495-498: The nil-safe comparison for CEL fallback data is wrong:
when both oldCELData and newCELData are nil the current isEqualToData: call
returns NO and triggers an unnecessary cache flush. Update the conditional
around newCELData/oldCELData (produced by [SNTCELFallbackRule serializeArray:])
to treat nil==nil as equal — e.g., only flush via
self.flushCacheBlock(FlushCacheMode::kAllCaches,
FlushCacheReason::kCELFallbackRulesChanged) when the two pointers are not both
nil and are not equal (use an explicit nil check combined with isEqualToData:),
ensuring the atomic path won’t flush caches on clean syncs.
---
Nitpick comments:
In `@Source/santasyncservice/SNTSyncConfigBundle.mm`:
- Around line 72-76: Update the explanatory comment above the bundle.syncType
assignment (the block that mentions SyncTypeRequired/Normal) to reflect the new
behavior: the daemon no longer uses the atomic-reset trigger of syncType ==
@(SNTSyncTypeNormal); it now checks the clearSyncStateBeforeApply flag to decide
to clear persisted sync state before applying a clean-sync bundle. Mention that
bundle.syncType is still set to Normal for backwards compatibility but the
actual reset signal is clearSyncStateBeforeApply, and remove or correct the
legacy wording that implies the daemon keys off syncType alone.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2dca3faa-46a5-4bf1-9df5-f0be7c1b4f48
📒 Files selected for processing (12)
Source/common/SNTConfigBundle.hSource/common/SNTConfigBundle.mmSource/common/SNTConfigBundleTest.mmSource/common/SNTConfigurator.hSource/common/SNTConfigurator.mmSource/common/SNTConfiguratorTest.mmSource/common/SNTXPCControlInterface.hSource/santad/SNTDaemonControlController.mmSource/santasyncservice/SNTSyncConfigBundle.mmSource/santasyncservice/SNTSyncConfigBundleTest.mmSource/santasyncservice/SNTSyncManager.hSource/santasyncservice/SNTSyncTest.mm
✅ Files skipped from review due to trivial changes (1)
- Source/santasyncservice/SNTSyncManager.h
🚧 Files skipped from review as they are similar to previous changes (3)
- Source/common/SNTXPCControlInterface.h
- Source/common/SNTConfigurator.mm
- Source/santasyncservice/SNTSyncConfigBundleTest.mm
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/santasyncservice/SNTSyncConfigBundleTest.mm`:
- Around line 300-311: Add a symmetric test case to
testPostflightConfigBundleOmitsPushTokenChainWhenJWTsMissing: after the existing
checks, set syncState.pushJWT to a non-nil value while leaving
syncState.pushIssuerJWT nil and call PostflightConfigBundle(syncState) again,
then assert that bundle.pushTokenChain is nil; this mirrors the existing check
for pushIssuerJWT-only and ensures SNTSyncState + PostflightConfigBundle
correctly avoids returning a half-chain when only pushJWT or only pushIssuerJWT
is present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4952020-8fb7-47f0-bed0-477a4dcbb6dd
📒 Files selected for processing (1)
Source/santasyncservice/SNTSyncConfigBundleTest.mm
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Source/common/SNTConfigurator.mm (1)
2144-2148:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the full-sync → rule-sync timestamp coupling in the atomic path.
-setFullSyncLastSuccess:still updates both timestamps, but this replace path only writeskFullSyncLastSuccess. On clean/full postflight, the bundle setsfullSyncLastSuccesswithoutruleSyncLastSuccess, so the atomic swap can erase the rule-sync timestamp and make the daemon treat rules as never synced.Suggested fix
[bundle fullSyncLastSuccess:^(NSDate* v) { newSyncState[kFullSyncLastSuccess] = v; + newSyncState[kRuleSyncLastSuccess] = v; }]; [bundle ruleSyncLastSuccess:^(NSDate* v) { newSyncState[kRuleSyncLastSuccess] = v; }];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/common/SNTConfigurator.mm` around lines 2144 - 2148, The atomic replace path currently only writes kFullSyncLastSuccess and can drop the rule-sync timestamp; update the atomic swap to preserve the full-sync → rule-sync coupling by writing both kFullSyncLastSuccess and kRuleSyncLastSuccess when the bundle provides both values (or when setFullSyncLastSuccess is used) so newSyncState includes kRuleSyncLastSuccess alongside kFullSyncLastSuccess; locate the blocks/bindings that set newSyncState (the bundle fullSyncLastSuccess and ruleSyncLastSuccess handlers and the atomic swap that commits newSyncState) and ensure both keys are assigned together before the atomic commit.Source/santad/SNTDaemonControlController.mm (1)
495-497:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTreat nil CEL snapshots as equal in this comparison.
If both
serializeArray(...)calls returnnil,[oldCELData isEqualToData:newCELData]evaluates false via nil messaging, so every clean sync with no CEL fallback rules still flushes caches.Suggested fix
- if (![oldCELData isEqualToData:newCELData] && self.flushCacheBlock) { + if (oldCELData != newCELData && ![oldCELData isEqualToData:newCELData] && + self.flushCacheBlock) { self.flushCacheBlock(FlushCacheMode::kAllCaches, FlushCacheReason::kCELFallbackRulesChanged); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/santad/SNTDaemonControlController.mm` around lines 495 - 497, The comparison currently treats two nil snapshots as different because `[oldCELData isEqualToData:newCELData]` is called when both are nil; update the check around oldCELData/newCELData in SNTDaemonControlController.mm so that if both `oldCELData` and `newCELData` are nil the branch is considered equal and does not call `self.flushCacheBlock`, but if either is non-nil compare them with `isEqualToData:` and only call `self.flushCacheBlock(FlushCacheMode::kAllCaches, FlushCacheReason::kCELFallbackRulesChanged)` when they differ; reference `oldCELData`, `newCELData`, `serializeArray:`, and `flushCacheBlock` to locate and modify the conditional.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/santasyncservice/SNTSyncConfigBundle.mm`:
- Around line 72-76: The comment is outdated: the daemon no longer uses
bundle.syncType == @(SNTSyncTypeNormal) to signal a clean-sync; it now uses
clearSyncStateBeforeApply. Update the comment near the bundle.syncType
assignment (and references to syncState/SNTSyncTypeNormal) to state that we
explicitly set clearSyncStateBeforeApply to trigger wiping persisted sync state
before applying, removing any mention that the daemon keys off syncType ==
@(SNTSyncTypeNormal).
---
Duplicate comments:
In `@Source/common/SNTConfigurator.mm`:
- Around line 2144-2148: The atomic replace path currently only writes
kFullSyncLastSuccess and can drop the rule-sync timestamp; update the atomic
swap to preserve the full-sync → rule-sync coupling by writing both
kFullSyncLastSuccess and kRuleSyncLastSuccess when the bundle provides both
values (or when setFullSyncLastSuccess is used) so newSyncState includes
kRuleSyncLastSuccess alongside kFullSyncLastSuccess; locate the blocks/bindings
that set newSyncState (the bundle fullSyncLastSuccess and ruleSyncLastSuccess
handlers and the atomic swap that commits newSyncState) and ensure both keys are
assigned together before the atomic commit.
In `@Source/santad/SNTDaemonControlController.mm`:
- Around line 495-497: The comparison currently treats two nil snapshots as
different because `[oldCELData isEqualToData:newCELData]` is called when both
are nil; update the check around oldCELData/newCELData in
SNTDaemonControlController.mm so that if both `oldCELData` and `newCELData` are
nil the branch is considered equal and does not call `self.flushCacheBlock`, but
if either is non-nil compare them with `isEqualToData:` and only call
`self.flushCacheBlock(FlushCacheMode::kAllCaches,
FlushCacheReason::kCELFallbackRulesChanged)` when they differ; reference
`oldCELData`, `newCELData`, `serializeArray:`, and `flushCacheBlock` to locate
and modify the conditional.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e536573c-3354-497a-96eb-7a0527c5c97b
📒 Files selected for processing (14)
Source/common/BUILDSource/common/SNTConfigBundle.hSource/common/SNTConfigBundle.mmSource/common/SNTConfigBundleTest.mmSource/common/SNTConfigurator.hSource/common/SNTConfigurator.mmSource/common/SNTConfiguratorTest.mmSource/common/SNTXPCControlInterface.hSource/santactl/Commands/SNTCommandSync.mmSource/santad/SNTDaemonControlController.mmSource/santasyncservice/SNTSyncConfigBundle.mmSource/santasyncservice/SNTSyncConfigBundleTest.mmSource/santasyncservice/SNTSyncManager.hSource/santasyncservice/SNTSyncTest.mm
mlw
requested changes
May 7, 2026
| } | ||
| } | ||
|
|
||
| - (void)atomicallyApplyBundle:(SNTConfigBundle*)bundle { |
Contributor
There was a problem hiding this comment.
We shouldn't duplicate all the logic from the individual setSyncServer* methods.
| // TODO: Start a timer to flush the state to disk. On startup, Santa should | ||
| // check for the presence of the state file and, if no SyncBaseURL is | ||
| // configured, start the timer to clear sync state and flush to disk. | ||
| void (^block)(void) = ^{ |
Contributor
There was a problem hiding this comment.
nit: we should add an "is authorized" check:
if (!self.syncStateAccessAuthorizerBlock()) {
return;
}
| newSyncState[kExportConfigurationKey] = [v serialize]; | ||
| }]; | ||
| [bundle fullSyncLastSuccess:^(NSDate* v) { | ||
| newSyncState[kFullSyncLastSuccess] = v; |
Contributor
There was a problem hiding this comment.
Discrepancy - the rule sync last success is also set on the setter side, but not here.
|
|
||
| // Side-effects pass. Persistence is already done by atomicallyApplyBundle: | ||
| // for every slot except modeTransition. | ||
| [result modeTransition:^(SNTModeTransition* val) { |
Contributor
There was a problem hiding this comment.
This will result in not being part of the atomic swap. I don't think there are any KVO watchers for it today, but if any were added in the future it would result in unexpected fires.
Contributor
Author
|
Closing this in favor of #944 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes SNT-357