Compare commits

...

2 Commits

Author SHA1 Message Date
amir 3d7a8b5f3d fix: remote deletions not mirrored when file has no state record
When a file was uploaded before state-tracking worked (getFileMetadata was
broken), its SyncFileStateEntity was never saved. On next sync the engine
saw !local + remote + known=null and downloaded it back instead of deleting
it remotely, creating an infinite re-download loop.

Fix: syncDecide() now accepts hasPriorSyncState (derived from whether the
pair has any known states at all). On initial sync (no prior state) unknown
remote files are downloaded as before. Once the pair has been synced, unknown
remote-only files are treated as mirror-eligible deletions — same as if known
state existed — so locally-deleted files propagate to the remote correctly.

Verified live: 3 remote-only orphan files deleted from Nextcloud on sync.
Bump version to 1.0.12 (code 13).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-24 02:18:27 +00:00
amir 1d6a80e43d fix: SAF delete crash, getFileMetadata drop-first, MKCOL before upload
- LocalAccessor.Saf.delete() now uses docIdCache (same as openInputStream)
  and catches IllegalStateException from DocumentsContract.deleteDocument
  instead of propagating it through awaitAll() and crashing the whole sync
- WebDavProvider.getFileMetadata() passes dropFirst=false to parsePropfind
  since Depth:0 returns exactly 1 result (the file); drop(1) was discarding it
- SyncEngine.performSync() calls ensureRemoteDirs() before each upload so
  MKCOL is issued for any missing parent directories (405=exists is success)
- Bump version to 1.0.11 (code 12)

Verified against live Nextcloud: baseline ↑0 ↓0 ✗0, upload detection ↑1 ↓0 ✗0,
download detection ↑0 ↓1 ✗0.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-24 01:07:54 +00:00
5 changed files with 107 additions and 24 deletions
@@ -132,7 +132,10 @@ open class WebDavProvider(protected val account: CloudAccount) : CloudProvider {
val req = Request.Builder().url(url(remotePath)).method("PROPFIND", PROPFIND_BODY).header("Depth", "0").build()
client.newCall(req).execute().use { resp ->
if (resp.code != 207) throw Exception("HTTP ${resp.code}")
parsePropfind(resp.body?.string() ?: "", remotePath.substringBeforeLast('/'))
// Depth:0 returns exactly the requested resource as the single response entry.
// parsePropfind normally drops the first entry (the parent dir) for Depth:1
// directory listings, so pass dropFirst=false here.
parsePropfind(resp.body?.string() ?: "", remotePath.substringBeforeLast('/'), dropFirst = false)
.firstOrNull() ?: throw Exception("File not found")
}
}
@@ -153,7 +156,7 @@ open class WebDavProvider(protected val account: CloudAccount) : CloudProvider {
protected fun url(path: String) = "$baseUrl/${path.trimStart('/')}"
private fun parsePropfind(xml: String, parentPath: String): List<RemoteFile> {
private fun parsePropfind(xml: String, parentPath: String, dropFirst: Boolean = true): List<RemoteFile> {
val results = mutableListOf<RemoteFile>()
try {
val factory = XmlPullParserFactory.newInstance()
@@ -192,7 +195,7 @@ open class WebDavProvider(protected val account: CloudAccount) : CloudProvider {
eventType = parser.next()
}
} catch (_: Exception) {}
return results.drop(1) // drop the parent folder itself
return if (dropFirst) results.drop(1) else results
}
private fun parseHttpDate(value: String): Instant = try {
@@ -63,7 +63,13 @@ sealed class LocalAccessor {
class Saf(private val treeUri: Uri, private val resolver: ContentResolver) : LocalAccessor() {
// Populated by walkFiles so openInputStream can skip the re-query for files that
// already exist locally (uploads). Root-level files are the common failure case
// when findDocUri re-queries: the cache sidesteps the issue entirely.
private val docIdCache = mutableMapOf<String, String>()
override fun walkFiles(pair: SyncPair): Map<String, LocalFileInfo> {
docIdCache.clear()
val rootDocId = DocumentsContract.getTreeDocumentId(treeUri)
val childrenUri = DocumentsContract.buildChildDocumentsUriUsingTree(treeUri, rootDocId)
return cursorWalk(childrenUri, "", pair)
@@ -110,6 +116,7 @@ sealed class LocalAccessor {
if (ext in excludeExts) continue
if (size !in minBytes..maxBytes) continue
result[rel] = LocalFileInfo(rel, size, modified)
docIdCache[rel] = docId
}
}
}
@@ -117,7 +124,10 @@ sealed class LocalAccessor {
}
override fun openInputStream(relativePath: String): InputStream? {
val docUri = findDocUri(relativePath) ?: return null
val docUri = docIdCache[relativePath]
?.let { DocumentsContract.buildDocumentUriUsingTree(treeUri, it) }
?: findDocUri(relativePath)
?: return null
return resolver.openInputStream(docUri)
}
@@ -155,8 +165,15 @@ sealed class LocalAccessor {
}
override fun delete(relativePath: String): Boolean {
val docUri = findDocUri(relativePath) ?: return false
return DocumentsContract.deleteDocument(resolver, docUri)
val docUri = docIdCache[relativePath]
?.let { DocumentsContract.buildDocumentUriUsingTree(treeUri, it) }
?: findDocUri(relativePath)
?: return false
return try {
DocumentsContract.deleteDocument(resolver, docUri)
} catch (e: Exception) {
false
}
}
override fun lastModifiedMs(relativePath: String): Long {
@@ -70,6 +70,7 @@ class SyncEngine @Inject constructor(
val localFiles = accessor.walkFiles(pair)
val allPaths = (localFiles.keys + remoteFiles.keys + knownStates.keys).toSet()
val hasPriorSyncState = knownStates.isNotEmpty()
val semaphore = Semaphore(4)
// Each async block returns its outcome; no shared mutable state across coroutines.
@@ -87,13 +88,15 @@ class SyncEngine @Inject constructor(
val local = localFiles[rel]
val remote = remoteFiles[rel]
val known = knownStates[rel]
val decision = syncDecide(pair.syncDirection, pair.conflictStrategy, pair.deleteBehavior, local, remote, known)
val decision = syncDecide(pair.syncDirection, pair.conflictStrategy, pair.deleteBehavior, local, remote, known, hasPriorSyncState)
when (decision) {
SyncDecision.UPLOAD -> {
var uploadedRemoteFile: RemoteFile? = null
val bytes = runCatching {
ensureRemoteDirs(provider, pair.remotePath, rel)
accessor.openInputStream(rel)?.use { stream ->
provider.uploadFile(stream, "${pair.remotePath}/$rel", local!!.sizeBytes) { }
uploadedRemoteFile = provider.uploadFile(stream, "${pair.remotePath}/$rel", local!!.sizeBytes) { }.getOrThrow()
}
local!!.sizeBytes
}.getOrElse { e ->
@@ -102,10 +105,8 @@ class SyncEngine @Inject constructor(
return@withPermit FileOutcome(failed = 1)
}
logEvent(pair.id, SyncEventType.FILE_UPLOADED, rel, null, bytes)
// 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))
newState = buildState(pair.id, rel, local!!, remoteAfterTransfer = uploadedRemoteFile))
}
SyncDecision.DOWNLOAD -> {
val bytes = runCatching {
@@ -154,13 +155,15 @@ class SyncEngine @Inject constructor(
FileOutcome(conflicts = 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) {
// Save state whenever both sides are present and state is absent or
// incomplete (post-upload null metadata). Without a baseline record,
// a subsequent local deletion would look like an unseen remote file
// and be re-downloaded instead of triggering DELETE_REMOTE.
val saveState = local != null && remote != null && (
known == null ||
known.remoteModifiedAt == null || known.localModifiedAt == null
)
if (saveState) {
FileOutcome(skipped = 1, newState = buildState(pair.id, rel, local, remoteAfterTransfer = remote))
} else {
FileOutcome(skipped = 1)
@@ -184,6 +187,17 @@ class SyncEngine @Inject constructor(
)
}
private suspend fun ensureRemoteDirs(provider: CloudProvider, remotePairPath: String, rel: String) {
val parts = rel.replace('\\', '/').split('/')
var currentPath = remotePairPath
for (part in parts.dropLast(1)) {
currentPath = "$currentPath/$part"
provider.createDirectory(currentPath).onFailure { e ->
Timber.w("MKCOL $currentPath: ${e.message}")
}
}
}
private fun buildState(
pairId: Long,
rel: String,
@@ -215,6 +229,7 @@ internal fun syncDecide(
local: LocalFileInfo?,
remote: RemoteFile?,
known: SyncFileStateEntity?,
hasPriorSyncState: Boolean = false,
): SyncDecision {
val localExists = local != null
val remoteExists = remote != null
@@ -244,9 +259,21 @@ internal fun syncDecide(
}
!localExists && remoteExists -> when {
known == null -> when (direction) {
SyncDirection.DOWNLOAD_ONLY, SyncDirection.TWO_WAY -> SyncDecision.DOWNLOAD
else -> SyncDecision.SKIP
known == null -> if (!hasPriorSyncState) {
// Initial sync: no history at all — remote files are new, download them.
when (direction) {
SyncDirection.DOWNLOAD_ONLY, SyncDirection.TWO_WAY -> SyncDecision.DOWNLOAD
else -> SyncDecision.SKIP
}
} else {
// Pair has been synced before but this file has no state record
// (e.g. uploaded before state-tracking was fixed). Treat the same
// as a known remote-deletion: apply mirror/keep behavior.
when {
deleteBehavior == DeleteBehavior.KEEP -> SyncDecision.SKIP
direction == SyncDirection.UPLOAD_ONLY || direction == SyncDirection.TWO_WAY -> SyncDecision.DELETE_REMOTE
else -> SyncDecision.SKIP
}
}
else -> when {
deleteBehavior == DeleteBehavior.KEEP -> SyncDecision.SKIP
@@ -35,7 +35,8 @@ class SyncDecideTest {
dir: SyncDirection = SyncDirection.TWO_WAY,
conflict: ConflictStrategy = ConflictStrategy.KEEP_NEWEST,
delete: DeleteBehavior = DeleteBehavior.MIRROR,
) = syncDecide(dir, conflict, delete, local, remote, known)
hasPriorState: Boolean = known != null,
) = syncDecide(dir, conflict, delete, local, remote, known, hasPriorState)
// ── first sync (no known state) ───────────────────────────────────────────
@@ -116,6 +117,41 @@ class SyncDecideTest {
assertEquals(SyncDecision.SKIP,
decide(local(), null, state(), dir = SyncDirection.UPLOAD_ONLY))
@Test fun `local deleted TWO_WAY MIRROR deletes remote`() =
assertEquals(SyncDecision.DELETE_REMOTE, decide(null, remote(), state(), delete = DeleteBehavior.MIRROR))
@Test fun `local deleted TWO_WAY KEEP skips`() =
assertEquals(SyncDecision.SKIP, decide(null, remote(), state(), delete = DeleteBehavior.KEEP))
@Test fun `local deleted DOWNLOAD_ONLY skips remote deletion`() =
assertEquals(SyncDecision.SKIP,
decide(null, remote(), state(), dir = SyncDirection.DOWNLOAD_ONLY))
// ── local deleted, no state record (uploaded in broken version) ──────────
@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,
decide(null, remote(), known = null, delete = DeleteBehavior.MIRROR, hasPriorState = true))
@Test fun `initial sync remote only no prior state downloads`() =
assertEquals(SyncDecision.DOWNLOAD,
decide(null, remote(), known = null, hasPriorState = false))
// ── first-seen SKIP saves baseline so later deletions are detected ────────
@Test fun `first sync both exist same mtime uploads local wins tie`() =
assertEquals(SyncDecision.UPLOAD, decide(local(MS), remote(MS, etag = "abc")))
@Test fun `after first-seen skip local deleted deletes remote`() {
// Simulate: first sync saw both sides identical → SKIP (state saved by engine).
// Then local file deleted → known is now present → DELETE_REMOTE.
val known = state(localMs = MS, remoteMs = MS, etag = "abc")
assertEquals(SyncDecision.DELETE_REMOTE,
decide(null, remote(MS, etag = "abc"), known, delete = DeleteBehavior.MIRROR))
}
// ── directions ────────────────────────────────────────────────────────────
@Test fun `UPLOAD_ONLY ignores remote changes`() =
+2 -2
View File
@@ -1,2 +1,2 @@
VERSION_NAME=1.0.6
VERSION_CODE=7
VERSION_NAME=1.0.12
VERSION_CODE=13