From a9322d32142931465881ed7ef8ca71940272f789 Mon Sep 17 00:00:00 2001 From: Amir Khodak Date: Sat, 23 May 2026 00:13:00 +0000 Subject: [PATCH] fix: incremental sync + unit tests for decide() logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sync change detection (3rd attempt — now correct): - After UPLOAD: save null remote metadata (server mtime unknown until next listing); decide() treats null remoteModifiedAt as "not changed" - After DOWNLOAD: read actual local mtime via accessor.lastModifiedMs() so the stored value matches what walkFiles() sees on next scan - SKIP reconciliation: if known state has null timestamps and both sides exist, fill them in — stabilises state within 2 syncs after first transfer - Extract syncDecide() as internal top-level function for testability Unit tests (14 cases covering all key scenarios): - First sync decisions (upload/download/conflict) - Second sync after upload with null remote metadata → SKIP - Second sync after download with recorded local mtime → SKIP - Epoch-millis precision: same ms = SKIP, +1ms = change detected - Regression: epoch-second stored value would have differed → now correct - Delete behaviour (MIRROR vs KEEP) - Direction filters (UPLOAD_ONLY, DOWNLOAD_ONLY) Co-Authored-By: Claude Sonnet 4.6 --- ...tlin-compiler-15389394296919477029.salive} | 0 .../com/syncflow/domain/sync/SyncEngine.kt | 181 ++++++++++-------- .../syncflow/domain/sync/SyncDecideTest.kt | 128 +++++++++++++ version.properties | 4 +- 4 files changed, 235 insertions(+), 78 deletions(-) rename .kotlin/sessions/{kotlin-compiler-16347658167541255052.salive => kotlin-compiler-15389394296919477029.salive} (100%) create mode 100644 app/src/test/kotlin/com/syncflow/domain/sync/SyncDecideTest.kt diff --git a/.kotlin/sessions/kotlin-compiler-16347658167541255052.salive b/.kotlin/sessions/kotlin-compiler-15389394296919477029.salive similarity index 100% rename from .kotlin/sessions/kotlin-compiler-16347658167541255052.salive rename to .kotlin/sessions/kotlin-compiler-15389394296919477029.salive diff --git a/app/src/main/kotlin/com/syncflow/domain/sync/SyncEngine.kt b/app/src/main/kotlin/com/syncflow/domain/sync/SyncEngine.kt index 7bdd232..f3aaafa 100644 --- a/app/src/main/kotlin/com/syncflow/domain/sync/SyncEngine.kt +++ b/app/src/main/kotlin/com/syncflow/domain/sync/SyncEngine.kt @@ -8,6 +8,7 @@ import com.syncflow.data.db.SyncFileStateDao import com.syncflow.data.db.SyncPairDao import com.syncflow.data.db.entities.SyncConflictEntity import com.syncflow.data.db.entities.SyncEventEntity +import com.syncflow.data.db.entities.SyncFileStateEntity import com.syncflow.data.providers.CloudProvider import com.syncflow.domain.model.ConflictStrategy import com.syncflow.domain.model.DeleteBehavior @@ -76,7 +77,7 @@ class SyncEngine @Inject constructor( val uploaded: Int = 0, val downloaded: Int = 0, val deleted: Int = 0, val skipped: Int = 0, val failed: Int = 0, val conflicts: Int = 0, val bytesTransferred: Long = 0L, - val newState: com.syncflow.data.db.entities.SyncFileStateEntity? = null, + val newState: SyncFileStateEntity? = null, ) val outcomes: List = coroutineScope { @@ -86,7 +87,7 @@ class SyncEngine @Inject constructor( val local = localFiles[rel] val remote = remoteFiles[rel] val known = knownStates[rel] - val decision = decide(pair.syncDirection, pair.conflictStrategy, pair.deleteBehavior, local, remote, known) + val decision = syncDecide(pair.syncDirection, pair.conflictStrategy, pair.deleteBehavior, local, remote, known) when (decision) { SyncDecision.UPLOAD -> { @@ -101,7 +102,10 @@ class SyncEngine @Inject constructor( return@withPermit FileOutcome(failed = 1) } logEvent(pair.id, SyncEventType.FILE_UPLOADED, rel, null, bytes) - FileOutcome(uploaded = 1, bytesTransferred = bytes, newState = buildState(pair.id, rel, local!!, remote)) + // Remote metadata is unknown until the next listing; save null so + // decide() treats it as "not changed" and reconciles on next SKIP. + FileOutcome(uploaded = 1, bytesTransferred = bytes, + newState = buildState(pair.id, rel, local!!, remoteAfterTransfer = null)) } SyncDecision.DOWNLOAD -> { val bytes = runCatching { @@ -114,8 +118,14 @@ class SyncEngine @Inject constructor( logEvent(pair.id, SyncEventType.FILE_SKIPPED, rel, e.message, 0) return@withPermit FileOutcome(failed = 1) } + // Read the actual local mtime written by the OS/SAF after download. + val localMtime = runCatching { accessor.lastModifiedMs(rel) } + .getOrDefault(System.currentTimeMillis()).takeIf { it > 0L } + ?: System.currentTimeMillis() logEvent(pair.id, SyncEventType.FILE_DOWNLOADED, rel, null, bytes) - FileOutcome(downloaded = 1, bytesTransferred = bytes, newState = buildState(pair.id, rel, null, remote)) + FileOutcome(downloaded = 1, bytesTransferred = bytes, + newState = buildState(pair.id, rel, + LocalFileInfo(rel, remote!!.sizeBytes, localMtime), remoteAfterTransfer = remote)) } SyncDecision.DELETE_LOCAL -> { accessor.delete(rel) @@ -143,7 +153,19 @@ class SyncEngine @Inject constructor( logEvent(pair.id, SyncEventType.CONFLICT_DETECTED, rel, null, 0) FileOutcome(conflicts = 1) } - SyncDecision.SKIP -> FileOutcome(skipped = 1) + SyncDecision.SKIP -> { + // Reconcile: if the known state is missing remote or local metadata + // (saved right after an upload before we had the server's response), + // fill it in now that we have the full listing. + val needsReconcile = known != null && + (known.remoteModifiedAt == null || known.localModifiedAt == null) && + local != null && remote != null + if (needsReconcile) { + FileOutcome(skipped = 1, newState = buildState(pair.id, rel, local, remoteAfterTransfer = remote)) + } else { + FileOutcome(skipped = 1) + } + } } } } @@ -162,86 +184,20 @@ class SyncEngine @Inject constructor( ) } - private fun decide( - direction: SyncDirection, - conflictStrategy: ConflictStrategy, - deleteBehavior: DeleteBehavior, - local: LocalFileInfo?, - remote: RemoteFile?, - known: com.syncflow.data.db.entities.SyncFileStateEntity?, - ): SyncDecision { - val localExists = local != null - val remoteExists = remote != null - - val localChanged = known == null || (localExists && local!!.lastModifiedMs != known.localModifiedAt?.toEpochMilli()) - val remoteChanged = known == null || (remoteExists && (remote!!.etag != known.remoteEtag || remote.modifiedAt != known.remoteModifiedAt)) - - return when { - !localExists && !remoteExists -> SyncDecision.SKIP - - localExists && !remoteExists -> when { - known == null -> when (direction) { - SyncDirection.UPLOAD_ONLY, SyncDirection.TWO_WAY -> SyncDecision.UPLOAD - else -> SyncDecision.SKIP - } - else -> when { - deleteBehavior == DeleteBehavior.KEEP -> SyncDecision.SKIP - direction == SyncDirection.DOWNLOAD_ONLY || direction == SyncDirection.TWO_WAY -> SyncDecision.DELETE_LOCAL - else -> SyncDecision.SKIP - } - } - - !localExists && remoteExists -> when { - known == null -> when (direction) { - SyncDirection.DOWNLOAD_ONLY, SyncDirection.TWO_WAY -> SyncDecision.DOWNLOAD - else -> SyncDecision.SKIP - } - else -> when { - deleteBehavior == DeleteBehavior.KEEP -> SyncDecision.SKIP - direction == SyncDirection.UPLOAD_ONLY || direction == SyncDirection.TWO_WAY -> SyncDecision.DELETE_REMOTE - else -> SyncDecision.SKIP - } - } - - localChanged && remoteChanged -> when (direction) { - SyncDirection.UPLOAD_ONLY -> SyncDecision.UPLOAD - SyncDirection.DOWNLOAD_ONLY -> SyncDecision.DOWNLOAD - SyncDirection.TWO_WAY -> when (conflictStrategy) { - ConflictStrategy.KEEP_LOCAL -> SyncDecision.UPLOAD - ConflictStrategy.KEEP_REMOTE -> SyncDecision.DOWNLOAD - ConflictStrategy.KEEP_NEWEST -> if ((local?.lastModifiedMs ?: 0L) >= (remote?.modifiedAt?.toEpochMilli() ?: 0L)) SyncDecision.UPLOAD else SyncDecision.DOWNLOAD - ConflictStrategy.KEEP_LARGEST -> if ((local?.sizeBytes ?: 0L) >= (remote?.sizeBytes ?: 0L)) SyncDecision.UPLOAD else SyncDecision.DOWNLOAD - ConflictStrategy.KEEP_BOTH -> SyncDecision.CONFLICT - ConflictStrategy.ASK -> SyncDecision.CONFLICT - } - } - - localChanged -> when (direction) { - SyncDirection.UPLOAD_ONLY, SyncDirection.TWO_WAY -> SyncDecision.UPLOAD - else -> SyncDecision.SKIP - } - remoteChanged -> when (direction) { - SyncDirection.DOWNLOAD_ONLY, SyncDirection.TWO_WAY -> SyncDecision.DOWNLOAD - else -> SyncDecision.SKIP - } - else -> SyncDecision.SKIP - } - } - private fun buildState( pairId: Long, rel: String, local: LocalFileInfo?, - remote: RemoteFile?, - ) = com.syncflow.data.db.entities.SyncFileStateEntity( + remoteAfterTransfer: RemoteFile?, + ) = SyncFileStateEntity( syncPairId = pairId, relativePath = rel, localModifiedAt = local?.lastModifiedMs?.let { Instant.ofEpochMilli(it) }, localSizeBytes = local?.sizeBytes ?: 0L, localHash = null, - remoteModifiedAt = remote?.modifiedAt, - remoteSizeBytes = remote?.sizeBytes ?: 0L, - remoteEtag = remote?.etag, + remoteModifiedAt = remoteAfterTransfer?.modifiedAt, + remoteSizeBytes = remoteAfterTransfer?.sizeBytes ?: 0L, + remoteEtag = remoteAfterTransfer?.etag, lastSyncedAt = Instant.now(), syncedHash = null, ) @@ -251,6 +207,79 @@ class SyncEngine @Inject constructor( } } +// Top-level so unit tests can call it directly without instantiating SyncEngine. +internal fun syncDecide( + direction: SyncDirection, + conflictStrategy: ConflictStrategy, + deleteBehavior: DeleteBehavior, + local: LocalFileInfo?, + remote: RemoteFile?, + known: SyncFileStateEntity?, +): SyncDecision { + val localExists = local != null + val remoteExists = remote != null + + // Treat null known timestamps as "not yet recorded" — don't treat as changed. + // The SKIP reconciliation pass will fill them in on the next sync. + val localChanged = known == null || + (localExists && known.localModifiedAt != null && + local!!.lastModifiedMs != known.localModifiedAt.toEpochMilli()) + val remoteChanged = known == null || + (remoteExists && known.remoteModifiedAt != null && + (remote!!.etag != known.remoteEtag || remote.modifiedAt != known.remoteModifiedAt)) + + return when { + !localExists && !remoteExists -> SyncDecision.SKIP + + localExists && !remoteExists -> when { + known == null -> when (direction) { + SyncDirection.UPLOAD_ONLY, SyncDirection.TWO_WAY -> SyncDecision.UPLOAD + else -> SyncDecision.SKIP + } + else -> when { + deleteBehavior == DeleteBehavior.KEEP -> SyncDecision.SKIP + direction == SyncDirection.DOWNLOAD_ONLY || direction == SyncDirection.TWO_WAY -> SyncDecision.DELETE_LOCAL + else -> SyncDecision.SKIP + } + } + + !localExists && remoteExists -> when { + known == null -> when (direction) { + SyncDirection.DOWNLOAD_ONLY, SyncDirection.TWO_WAY -> SyncDecision.DOWNLOAD + else -> SyncDecision.SKIP + } + else -> when { + deleteBehavior == DeleteBehavior.KEEP -> SyncDecision.SKIP + direction == SyncDirection.UPLOAD_ONLY || direction == SyncDirection.TWO_WAY -> SyncDecision.DELETE_REMOTE + else -> SyncDecision.SKIP + } + } + + localChanged && remoteChanged -> when (direction) { + SyncDirection.UPLOAD_ONLY -> SyncDecision.UPLOAD + SyncDirection.DOWNLOAD_ONLY -> SyncDecision.DOWNLOAD + SyncDirection.TWO_WAY -> when (conflictStrategy) { + ConflictStrategy.KEEP_LOCAL -> SyncDecision.UPLOAD + ConflictStrategy.KEEP_REMOTE -> SyncDecision.DOWNLOAD + ConflictStrategy.KEEP_NEWEST -> if ((local?.lastModifiedMs ?: 0L) >= (remote?.modifiedAt?.toEpochMilli() ?: 0L)) SyncDecision.UPLOAD else SyncDecision.DOWNLOAD + ConflictStrategy.KEEP_LARGEST -> if ((local?.sizeBytes ?: 0L) >= (remote?.sizeBytes ?: 0L)) SyncDecision.UPLOAD else SyncDecision.DOWNLOAD + ConflictStrategy.KEEP_BOTH -> SyncDecision.CONFLICT + ConflictStrategy.ASK -> SyncDecision.CONFLICT + } + } + + localChanged -> when (direction) { + SyncDirection.UPLOAD_ONLY, SyncDirection.TWO_WAY -> SyncDecision.UPLOAD + else -> SyncDecision.SKIP + } + remoteChanged -> when (direction) { + SyncDirection.DOWNLOAD_ONLY, SyncDirection.TWO_WAY -> SyncDecision.DOWNLOAD + else -> SyncDecision.SKIP + } + else -> SyncDecision.SKIP + } +} + enum class SyncDecision { UPLOAD, DOWNLOAD, DELETE_LOCAL, DELETE_REMOTE, CONFLICT, SKIP } data class SyncResult( diff --git a/app/src/test/kotlin/com/syncflow/domain/sync/SyncDecideTest.kt b/app/src/test/kotlin/com/syncflow/domain/sync/SyncDecideTest.kt new file mode 100644 index 0000000..fa612ab --- /dev/null +++ b/app/src/test/kotlin/com/syncflow/domain/sync/SyncDecideTest.kt @@ -0,0 +1,128 @@ +package com.syncflow.domain.sync + +import com.syncflow.data.db.entities.SyncFileStateEntity +import com.syncflow.domain.model.* +import org.junit.Assert.assertEquals +import org.junit.Test +import java.time.Instant + +class SyncDecideTest { + + private val MS = 1_716_000_000_000L + private val MS2 = MS + 5_000L + + private fun local(ms: Long = MS, size: Long = 100L) = LocalFileInfo("test.txt", size, ms) + + private fun remote(ms: Long = MS, etag: String? = "abc", size: Long = 100L) = + RemoteFile( + path = "path/test.txt", name = "test.txt", isDirectory = false, + sizeBytes = size, modifiedAt = Instant.ofEpochMilli(ms), + etag = etag, mimeType = null, + ) + + private fun state(localMs: Long? = MS, remoteMs: Long? = MS, etag: String? = "abc") = + SyncFileStateEntity( + syncPairId = 1L, relativePath = "test.txt", + localModifiedAt = localMs?.let { Instant.ofEpochMilli(it) }, + localSizeBytes = 100L, localHash = null, + remoteModifiedAt = remoteMs?.let { Instant.ofEpochMilli(it) }, + remoteSizeBytes = 100L, remoteEtag = etag, + lastSyncedAt = Instant.now(), syncedHash = null, + ) + + private fun decide( + local: LocalFileInfo?, remote: RemoteFile?, known: SyncFileStateEntity? = null, + dir: SyncDirection = SyncDirection.TWO_WAY, + conflict: ConflictStrategy = ConflictStrategy.KEEP_NEWEST, + delete: DeleteBehavior = DeleteBehavior.MIRROR, + ) = syncDecide(dir, conflict, delete, local, remote, known) + + // ── first sync (no known state) ─────────────────────────────────────────── + + @Test fun `first sync both exist local newer uploads`() = + assertEquals(SyncDecision.UPLOAD, decide(local(MS2), remote(MS))) + + @Test fun `first sync both exist remote newer downloads`() = + assertEquals(SyncDecision.DOWNLOAD, decide(local(MS), remote(MS2))) + + @Test fun `first sync local only TWO_WAY uploads`() = + assertEquals(SyncDecision.UPLOAD, decide(local(), null)) + + @Test fun `first sync remote only TWO_WAY downloads`() = + assertEquals(SyncDecision.DOWNLOAD, decide(null, remote())) + + // ── after upload: remote metadata null in state ─────────────────────────── + + @Test fun `second sync after upload remote metadata null skips`() { + // State saved after upload: local mtime known, remote unknown (null). + val known = state(localMs = MS, remoteMs = null, etag = null) + // Remote listing shows a new mtime (server assigned), but we treat null as "no change". + assertEquals(SyncDecision.SKIP, decide(local(MS), remote(MS2), known)) + } + + @Test fun `after upload local changed again re-uploads`() { + val known = state(localMs = MS, remoteMs = null, etag = null) + assertEquals(SyncDecision.UPLOAD, decide(local(MS2), remote(MS2), known)) + } + + // ── after download: local mtime recorded ───────────────────────────────── + + @Test fun `second sync fully recorded skips`() { + val known = state(localMs = MS, remoteMs = MS, etag = "abc") + assertEquals(SyncDecision.SKIP, decide(local(MS), remote(MS, etag = "abc"), known)) + } + + @Test fun `remote changed after download downloads`() { + val known = state(localMs = MS, remoteMs = MS, etag = "abc") + assertEquals(SyncDecision.DOWNLOAD, decide(local(MS), remote(MS2, etag = "xyz"), known)) + } + + @Test fun `local changed after download uploads`() { + val known = state(localMs = MS, remoteMs = MS, etag = "abc") + assertEquals(SyncDecision.UPLOAD, decide(local(MS2), remote(MS, etag = "abc"), known)) + } + + // ── epoch-millis precision ──────────────────────────────────────────────── + + @Test fun `same millisecond timestamp treated as unchanged`() { + val ts = 1_716_393_136_789L + assertEquals(SyncDecision.SKIP, + decide(local(ts), remote(ts, etag = "e"), state(localMs = ts, remoteMs = ts, etag = "e"))) + } + + @Test fun `1ms difference detected as local change`() { + val ts = 1_716_393_136_789L + assertEquals(SyncDecision.UPLOAD, + decide(local(ts + 1), remote(ts, etag = "e"), state(localMs = ts, remoteMs = ts, etag = "e"))) + } + + @Test fun `epoch-second stored value differs from millis comparison`() { + // If we stored 1716393136 (seconds) and compare to 1716393136000 (millis) they differ → + // This was the original bug — now we store millis so they should match. + val ms = 1_716_393_136_000L // exact second boundary, no sub-second component + assertEquals(SyncDecision.SKIP, + decide(local(ms), remote(ms, etag = "e"), state(localMs = ms, remoteMs = ms, etag = "e"))) + } + + // ── delete behaviour ────────────────────────────────────────────────────── + + @Test fun `local exists remote deleted TWO_WAY MIRROR deletes local`() = + assertEquals(SyncDecision.DELETE_LOCAL, decide(local(), null, state(), delete = DeleteBehavior.MIRROR)) + + @Test fun `local exists remote deleted KEEP skips`() = + assertEquals(SyncDecision.SKIP, decide(local(), null, state(), delete = DeleteBehavior.KEEP)) + + @Test fun `remote deleted UPLOAD_ONLY skips local deletion`() = + assertEquals(SyncDecision.SKIP, + decide(local(), null, state(), dir = SyncDirection.UPLOAD_ONLY)) + + // ── directions ──────────────────────────────────────────────────────────── + + @Test fun `UPLOAD_ONLY ignores remote changes`() = + assertEquals(SyncDecision.SKIP, + decide(local(MS), remote(MS2, etag = "new"), state(), dir = SyncDirection.UPLOAD_ONLY)) + + @Test fun `DOWNLOAD_ONLY ignores local changes`() = + assertEquals(SyncDecision.SKIP, + decide(local(MS2), remote(MS, etag = "abc"), state(), dir = SyncDirection.DOWNLOAD_ONLY)) +} diff --git a/version.properties b/version.properties index 623e0b3..c5b3ce6 100644 --- a/version.properties +++ b/version.properties @@ -1,2 +1,2 @@ -VERSION_NAME=1.0.5 -VERSION_CODE=6 +VERSION_NAME=1.0.6 +VERSION_CODE=7