sync: support batched mode for sync state and handle clean sync#944
Merged
Conversation
github-actions
Bot
added
configurator
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds a persisted clearSyncStateBeforeApply callback to SNTConfigBundle, implements performSyncStateBatch: in SNTConfigurator to buffer and atomically commit sync-state (single KVO + single disk write), updates daemon to apply sync updates inside a batch and defer CEL cache flush until after commit, and adds tests for these behaviors. ChangesSync-State Atomic Batching and Clear Flow
Sequence Diagram(s)sequenceDiagram
participant SNTSyncPostflight as SNTSyncPostflight
participant SNTConfigBundle as SNTConfigBundle
participant SNTDaemonControlController as DaemonController
participant SNTConfigurator as Configurator
participant Disk as Disk
SNTSyncPostflight->>SNTConfigBundle: create PostflightConfigBundle (may set clearSyncStateBeforeApply)
DaemonController->>SNTConfigurator: performSyncStateBatch(block)
SNTConfigurator->>SNTConfigurator: seed batchedSyncState from syncState
DaemonController->>SNTConfigurator: apply multiple syncState updates (writes go to batchedSyncState)
alt bundle requests clear before apply
SNTConfigBundle->>SNTConfigurator: invoke clearSyncStateBeforeApply(YES) -> clears persisted state (batch-aware)
end
SNTConfigurator->>SNTConfigurator: commit batchedSyncState -> syncState (single KVO)
SNTConfigurator->>Disk: atomic write sync-state plist
DaemonController->>SNTConfigurator: run post-commit mode transition enforcement
DaemonController->>DaemonController: serialize new CEL rules and flush caches if changed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
🤖 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 1986-1998: Wrap the call to block() in an Objective-C
`@try/`@finally (and optional `@catch` to log) so that self.batchedSyncState is
always cleared on exit: set self.batchedSyncState = self.syncState.mutableCopy
before the try, call block() inside `@try`, move the assignments self.syncState =
self.batchedSyncState and [self saveSyncStateToDisk] into the normal path, and
in `@finally` set self.batchedSyncState = nil (and if you add `@catch`, log the
exception with LOGE and rethrow). This ensures performSyncStateBatch:, and the
properties batchedSyncState/syncState and method saveSyncStateToDisk, do not
leave a stale batchedSyncState after an exception.
🪄 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: 2b90c4dc-7963-4d56-9c96-c41dfadca770
📒 Files selected for processing (10)
Source/common/SNTConfigBundle.hSource/common/SNTConfigBundle.mmSource/common/SNTConfigBundleTest.mmSource/common/SNTConfigurator.hSource/common/SNTConfigurator.mmSource/common/SNTConfiguratorTest.mmSource/santad/SNTDaemonControlController.mmSource/santasyncservice/SNTSyncConfigBundle.mmSource/santasyncservice/SNTSyncConfigBundleTest.mmSource/santasyncservice/SNTSyncTest.mm
tburgin
reviewed
May 11, 2026
Builds on #944. Three connected changes: 1. Pull modeTransition persistence into performSyncStateBatch so it commits atomically with the rest of sync-derived state. TMM's enforcement side effects (Revoke + GUI notify) run after the batch via NewModeTransitionReceived so Available(nil) reads consistent post-commit state. To avoid a second syncState KVO fire on the Revoke path, RevokeLocked skips its own setSyncServerModeTransition: when the current transition is already a revoke. Other Revoke entry points (sync URL change, etc.) keep their existing behavior. 2. performSyncStateBatch: and saveSyncStateToDisk return BOOL so callers can gate post-commit work on durable persistence. Used to skip the CEL cache flush on disk failure. Removed the @try/@finally wrapper around the batch block; runtime exceptions are fatal by design. 3. Drop the syncStateAccessAuthorizerBlock guard from clearSyncState. The production authorizer requires syncBaseURL != nil, but SNTSyncdQueue invokes clearSyncState precisely when syncBaseURL went to nil. Gating cleanup there would make it unreachable. Tests cover the BOOL return on disk failure and lock in clearSyncState's authorizer bypass.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Source/common/SNTConfiguratorTest.mm (1)
377-380: ⚡ Quick winAssert
performSyncStateBatch:succeeds in the clear-path test.This test validates disk/in-memory post-clear state, but it does not assert the API contract (
YESon successful durable commit) for this path.Suggested test tightening
- [cfg performSyncStateBatch:^{ + BOOL committed = [cfg performSyncStateBatch:^{ [cfg clearSyncState]; [cfg setSyncServerClientMode:SNTClientModeMonitor]; }]; + XCTAssertTrue(committed, @"Batch with clear + post-clear writes should commit successfully");🤖 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/SNTConfiguratorTest.mm` around lines 377 - 380, The test calls [cfg performSyncStateBatch:^{ ... }] but does not assert its return value; update the test to capture the BOOL result of performSyncStateBatch: and assert it equals YES to verify the durable commit succeeded (i.e. call performSyncStateBatch: and XCTAssertTrue(result) or equivalent). Ensure you reference performSyncStateBatch:, clearSyncState, and setSyncServerClientMode:SNTClientModeMonitor when adding the assertion so the success of that batch operation is enforced in the clear-path test.
🤖 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.
Nitpick comments:
In `@Source/common/SNTConfiguratorTest.mm`:
- Around line 377-380: The test calls [cfg performSyncStateBatch:^{ ... }] but
does not assert its return value; update the test to capture the BOOL result of
performSyncStateBatch: and assert it equals YES to verify the durable commit
succeeded (i.e. call performSyncStateBatch: and XCTAssertTrue(result) or
equivalent). Ensure you reference performSyncStateBatch:, clearSyncState, and
setSyncServerClientMode:SNTClientModeMonitor when adding the assertion so the
success of that batch operation is enforced in the clear-path test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca786f6e-921e-4311-8ea7-819a5592e70a
📒 Files selected for processing (5)
Source/common/SNTConfigurator.hSource/common/SNTConfigurator.mmSource/common/SNTConfiguratorTest.mmSource/santad/SNTDaemonControlController.mmSource/santad/TemporaryMonitorMode.mm
🚧 Files skipped from review as they are similar to previous changes (2)
- Source/santad/SNTDaemonControlController.mm
- Source/common/SNTConfigurator.mm
mlw
approved these changes
May 14, 2026
russellhancox
approved these changes
May 14, 2026
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
5 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.
Support batched mode for sync state and handle clean sync
Fixes SNT-357