From 62f9f015d6bad7cf586328daf34539f205ee9da1 Mon Sep 17 00:00:00 2001
From: Friday <amir.khodak@gmail.com>
Date: Fri, 5 Jun 2026 02:32:16 +0000
Subject: [PATCH] Fix two stale SyncDecideTest cases (CI never ran tests
 before)

These contradicted deliberate later safety fixes in syncDecide:
- sub-second mtime delta is now SKIP (second-precision comparison was the
  fix for the FAT32/WebDAV phantom-change sync loops), not UPLOAD. Added a
  full-second-delta case to keep change-detection coverage.
- remote file with no state record now DOWNLOADs instead of DELETE_REMOTE:
  known==null can't be distinguished from a brand-new remote file, so the
  engine never deletes on ambiguity. Genuinely-deleted local files still
  have a state record and route to DELETE_REMOTE.

All 25 unit tests pass; assembleRelease builds and signs cleanly (compileSdk 35).
---
 .../syncflow/domain/sync/SyncDecideTest.kt    | 25 +++++++++++++------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/app/src/test/kotlin/com/syncflow/domain/sync/SyncDecideTest.kt b/app/src/test/kotlin/com/syncflow/domain/sync/SyncDecideTest.kt
index d1b544e..391cfe7 100644
--- a/app/src/test/kotlin/com/syncflow/domain/sync/SyncDecideTest.kt
+++ b/app/src/test/kotlin/com/syncflow/domain/sync/SyncDecideTest.kt
@@ -91,10 +91,19 @@ class SyncDecideTest {
             decide(local(ts), remote(ts, etag = "e"), state(localMs = ts, remoteMs = ts, etag = "e")))
     }
 
-    @Test fun `1ms difference detected as local change`() {
+    @Test fun `sub-second mtime difference treated as unchanged`() {
+        // Second-precision comparison is intentional: FAT32 has 2s mtime resolution and WebDAV
+        // 1s, so sub-second deltas are phantom changes that caused rewrite loops. A 1ms diff
+        // within the same second must NOT be treated as a change.
+        val ts = 1_716_393_136_789L
+        assertEquals(SyncDecision.SKIP,
+            decide(local(ts + 1), remote(ts, etag = "e"), state(localMs = ts, remoteMs = ts, etag = "e")))
+    }
+
+    @Test fun `mtime change of a full second 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")))
+            decide(local(ts + 1000), remote(ts, etag = "e"), state(localMs = ts, remoteMs = ts, etag = "e")))
     }
 
     @Test fun `epoch-second stored value differs from millis comparison`() {
@@ -127,12 +136,14 @@ class SyncDecideTest {
         assertEquals(SyncDecision.SKIP,
             decide(null, remote(), state(), dir = SyncDirection.DOWNLOAD_ONLY))
 
-    // ── local deleted, no state record (uploaded in broken version) ──────────
+    // ── remote exists, no state record: never delete on ambiguity ────────────
 
-    @Test fun `local deleted no known state but pair has prior history deletes remote`() =
-        // hasPriorState=true means the pair has been synced before; file has no state
-        // because it was uploaded when getFileMetadata was broken. Should still mirror deletion.
-        assertEquals(SyncDecision.DELETE_REMOTE,
+    @Test fun `remote exists with no state record downloads rather than deleting`() =
+        // known=null can mean a brand-new remote file OR one whose state was lost. The engine
+        // cannot tell them apart, so it downloads rather than risk deleting a real file —
+        // worst case is a re-downloaded file, never a lost one. A file the user genuinely
+        // deleted locally still has its state record, which routes to DELETE_REMOTE.
+        assertEquals(SyncDecision.DOWNLOAD,
             decide(null, remote(), known = null, delete = DeleteBehavior.MIRROR, hasPriorState = true))
 
     @Test fun `initial sync remote only no prior state downloads`() =