[MS-1299] Sync architecture revamp, part 2

[alex-vt]

JIRA ticket
Will be released in: 2026.2.0

Boundaries for the changes - what's intentionally out of scope:

• WorkManager and workers. This is already the optimal foundation for sync - both foreground and background.
• SyncInfoViewModel. Already observes sync state.
• UI/UX. Nothing visually new.

Previously done in phase 1:

• ObserveSyncInfoUseCase made fully reactive - without suspension points.
• SyncUseCase, a unified (events + images) sync status reactive observation use case introduced. It is a StateFlow, with up-to-date sync status available as .value syncronously from anywhere.
• CountSyncableUseCase, a unified (events + images) counters reactive observation use case introduced.

Notable changes

• observeSyncState(): StateFlow<SyncStatus> introduced in SyncOrchestrator
• executeOneTime(command: OneTime): Job introduced in SyncOrchestrator
• executeSchedulingCommand(command: ScheduleCommand): Job introduced in SyncOrchestrator

Testing guidance

• Sync counters and buttons on dashboard, sync info and logout screens should work correctly.
• Some usecase calls were changes in other places - full regression testing recommended when the work is completed.

Additional work checklist

• Effect on other features and security has been considered
• Design document marked as "In development" (if applicable)
• External (Gitbook) and internal (Confluence) Documentation is up to date (or ticket created)
• Test cases in Testiny are up to date (or ticket created)
• Other teams notified about the changes (if applicable)

[Copilot]

Pull request overview

This PR implements phase 2 of a comprehensive sync architecture revamp, introducing a structured command-based API for sync operations. The changes replace the imperative sync orchestration with a reactive, command-driven approach using expressive DSL-style builders.

Changes:

• Introduced SyncCommands DSL builders with autocomplete-friendly structure for scheduling and one-time sync operations
• Created SyncResponse with await() extension for suspendable command execution
• Migrated most SyncOrchestrator and EventSyncManager functionality into ExecuteSyncCommandUseCase and updated SyncUseCase
• Updated all callsites throughout features (login-check, dashboard, validate-subject-pool) and Application.kt to use new sync commands

Reviewed changes

Copilot reviewed 39 out of 39 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
infra/sync/src/main/java/com/simprints/infra/sync/SyncCommands.kt	New DSL-based command builder infrastructure with expressive API
infra/sync/src/main/java/com/simprints/infra/sync/SyncResponse.kt	Response wrapper with suspend-aware await() for command completion
infra/sync/src/main/java/com/simprints/infra/sync/usecase/internal/ExecuteSyncCommandUseCase.kt	Core command execution logic extracted from SyncOrchestrator
infra/sync/src/main/java/com/simprints/infra/sync/usecase/SyncUseCase.kt	Updated to accept and execute sync commands
infra/sync/src/main/java/com/simprints/infra/sync/SyncOrchestratorImpl.kt	Slimmed down - removed methods migrated to ExecuteSyncCommandUseCase
infra/sync/src/main/java/com/simprints/infra/sync/SyncOrchestrator.kt	Interface reduced with methods moved to use cases
infra/sync/src/main/java/com/simprints/infra/sync/config/usecase/*.kt	Config use cases updated to use new sync commands with stopAndStartAround
id/src/main/java/com/simprints/id/Application.kt	Application startup now uses sync commands for background work scheduling
id/src/main/java/com/simprints/id/services/sync/events/down/EventDownSyncResetService.kt	Service updated to use sync commands with await()
feature/login-check/src/main/java/com/simprints/feature/logincheck/*.kt	Login flow uses sync commands with proper await() semantics
feature/dashboard/src/main/java/com/simprints/feature/dashboard/settings/syncinfo/*.kt	Dashboard sync UI migrated to sync commands
feature/validate-subject-pool/src/main/java/com/simprints/feature/validatepool/usecase/*.kt	Validation flow updated with sync commands
All test files	Comprehensive test coverage for new command infrastructure and updated existing tests

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
94.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[luhmirin-s]

I have couple of concerns regarding the complexity of this solution, as it does not seem to simplify sync usage compared to the original sync orchestration.

[BurningAXE]

What started out as an attempt to simplify sync VM implementation has spiralled into a general sync refactoring. Given that we are discussing architecture and not implementation in the PR - I think it makes sense to "go back to the drawing board" and actually pass this through a design doc. There we can discuss the pros and cons of different approaches. Once we agree on the architecture (which shouldn't necessarily change (much)), we can move to the implementation part.

[alex-vt]

What started out as an attempt to simplify sync VM implementation has spiralled into a general sync refactoring. Given that we are discussing architecture and not implementation in the PR - I think it makes sense to "go back to the drawing board" and actually pass this through a design doc. There we can discuss the pros and cons of different approaches. Once we agree on the architecture (which shouldn't necessarily change (much)), we can move to the implementation part.

Just to keep the scope already narrowed before re-planning: the part about the usage of workers isn't up to a change, at least not recommended - that was already researched before Phase 1. Workers are already the reliable solution and shouldn't be replaced, even if the app is in the foreground - at any moment it isn't anymore, and other solutions to keep the sync going would be no better. Also, the VM is already using reactive pattern as well. So, the part about reactive sync architecture focuses on the SyncOrchestrator.

Also, the Phase 1 had already outlined the work that's being done here. It was the exploration part, marked as "spike", and is now in main. Here we have the planned implementation of the SyncUseCase stub from Phase 1.

[luhmirin-s]

Left some minor nitpicks and suggestions, but overall this iteration LGTM - it is a nice balance of API improvements with manageable amount of complexity.

[luhmirin-s]

Nitpick: I don't feel that those variables provide anything meaningful.

Wouldn't this be simpler?

return appScope.launch(ioDispatcher) {
  when (action) {
    START -> start()
    STOP -> {
      stop()
      Job().apply { complete() }
    } 
    RESTART -> { 
      stop()
      start()
    }
  }
}

Also, looking at the logic, is there a case where we would want to explicitly call "START" instead of "RESTART" (to be sure there are no 2 jobs running at the same time)?

[alex-vt]

I'll take another look at these in the phase 3 intended for cleanup. Now we have the signatures stabilized and can proceed with this.

[luhmirin-s]

Could be just if (action == OneTime.Action.STOP) {} here.

[luhmirin-s]

This is literally if (action != ScheduleCommand.Action.RESCHEDULE), tho.

[luhmirin-s]

Nitpick, IMO it would be more readable without extra variable.

[luhmirin-s]

IMO, this is much clearer than the previous version.

[alex-vt]

Thanks! Let's keep the signature as it is (I followed your suggestion verbatim), for expressiveness.

[luhmirin-s]

I would also suggest to take advantage of polymorphism and define both methods as fun execute(...): Job with different parameter types to reduce a bit of duplication in the API:

syncOrchestrator.execute(OneTime.Events.stop())
syncOrchestrator.execute(ScheduleCommand.Events.unschedule())

[luhmirin-s]

Do I understand correctly, that "everything" in this case also includes the scheduled project/config sync? Are commands for those workers also in the works?

[alex-vt]

Yes, it already includes these.

[luhmirin-s]

What about the explicit commands to start/stop and schedule/unschedule the config sync workers separately?

[alex-vt]

Config sync wasn't planned to be touched here.

[Copilot]

Pull request overview

Copilot reviewed 42 out of 42 changed files in this pull request and generated 5 comments.

[Copilot]

The order of constructor parameters has been changed, placing syncOrchestrator after eventSyncManager and enrolmentRecordRepository, while it was first in the original constructor. While this doesn't affect functionality due to named parameters in Dagger injection, it breaks consistency with the pattern of having dependencies in a stable, logical order.

Consider maintaining the original parameter order to avoid confusion and maintain consistency with the codebase.

Suggested change

	private val eventSyncManager: EventSyncManager,
	private val enrolmentRecordRepository: EnrolmentRecordRepository,
	private val syncOrchestrator: SyncOrchestrator,
	private val syncOrchestrator: SyncOrchestrator,
	private val eventSyncManager: EventSyncManager,
	private val enrolmentRecordRepository: EnrolmentRecordRepository,

[Copilot]

The order of constructor parameters has been changed, placing syncOrchestrator after configRepository, while the test setup order at line 36 keeps the original order. This inconsistency can cause confusion during code review and maintenance.

Consider keeping constructor parameters in a consistent order across production and test code.

Suggested change

	private val configRepository: ConfigRepository,
	private val syncOrchestrator: SyncOrchestrator,
	private val syncOrchestrator: SyncOrchestrator,
	private val configRepository: ConfigRepository,