From de33c2d5e670146af5869e0866341b803e1c7bed Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 06:58:36 +0000 Subject: [PATCH] Garbage-collect orphaned attachment blobs and local files The app could upload attachment blobs but never delete them: removing an attachment, deleting an entry, or moving one to another calendar all left the uploaded file behind in its CalDAV collection (and local files behind on the device), so storage grew without bound. Adds a proper cleanup lifecycle: - New WebDAV deleteFile(url) primitive. - New PendingRemoteDeletionDto table (schema migration 13) that queues orphaned blob URLs independently of entries, so the ON DELETE CASCADE from an entry can't lose them before they're deleted. - SyncCoordinator drains the queue per calendar after pushing local changes, best-effort with retry (2xx or 404 clears the entry, anything else retries). Enqueue points: - Attachment removal (details screen) queues the removed blob. - Move queues the source-collection blobs - captured before re-creating the entries, since the copy's attachment rows reuse the same UNIQUE uid and replace the originals'. Only blobs that get re-uploaded into the target are queued; a remote-only attachment the copy still points at is left alone. - Trashbin hard-delete (deleteTrashed) queues each attachment's blob and deletes its local file before the rows cascade away. IcalEntryRepositoryImpl now depends on FileManager to reclaim local files. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_011XuDzYDVnD3s5Zzm7aJ3K3 --- .../screens/core/SyncCoordinator.kt | 19 ++++++ .../repository/IcalEntryRepositoryImpl.kt | 67 ++++++++++++++++++- .../data/webdav/RemoteDataSourceIcalEntry.kt | 11 +++ .../webdav/WebDavRemoteIcalEntryDataSource.kt | 4 ++ .../core/domain/PendingRemoteFileDeletion.kt | 11 +++ .../domain/repository/IcalEntryRepository.kt | 7 ++ .../details/presentation/DetailsViewModel.kt | 8 ++- .../at/techbee/spectacled/sqldelight/13.sqm | 7 ++ .../spectacled/sqldelight/icalentry_dto.sq | 3 + .../sqldelight/pending_remote_deletion_dto.sq | 21 ++++++ .../screens/core/SyncCoordinatorTest.kt | 7 ++ 11 files changed, 161 insertions(+), 4 deletions(-) create mode 100644 shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/domain/PendingRemoteFileDeletion.kt create mode 100644 shared/src/commonMain/sqldelight/at/techbee/spectacled/sqldelight/13.sqm create mode 100644 shared/src/commonMain/sqldelight/at/techbee/spectacled/sqldelight/pending_remote_deletion_dto.sq diff --git a/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/SyncCoordinator.kt b/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/SyncCoordinator.kt index 8e89abe5..932644f4 100644 --- a/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/SyncCoordinator.kt +++ b/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/SyncCoordinator.kt @@ -27,6 +27,7 @@ import io.ktor.client.plugins.ClientRequestException import io.ktor.client.plugins.HttpRequestTimeoutException import io.ktor.client.plugins.ResponseException import io.ktor.client.plugins.ServerResponseException +import io.ktor.http.HttpStatusCode import io.ktor.http.Url import io.ktor.http.isSuccess import kotlinx.coroutines.coroutineScope @@ -202,6 +203,7 @@ class SyncCoordinator( syncCollectionResponse.hrefs.mapNotNull { (url, eTag) -> eTag?.let { url to it } }.toMap() ) pushLocalChanges(calendar) + drainPendingRemoteFileDeletions(calendar) calendarRepository.updateCalendarSyncStatus( CalendarSyncStatus(CalendarSyncStatusType.SYNCED).serialize(), syncCollectionResponse.syncToken, @@ -306,6 +308,7 @@ class SyncCoordinator( is MultigetResourceHrefETagResult.Success -> { applyServerchanges(calendar, multigetResourceHrefsMultiplatformResult.hrefs) pushLocalChanges(calendar) + drainPendingRemoteFileDeletions(calendar) calendarRepository.updateCalendarSyncStatus( CalendarSyncStatus(CalendarSyncStatusType.SYNCED).serialize(), multigetResourceHrefsMultiplatformResult.syncToken, @@ -439,6 +442,22 @@ class SyncCoordinator( dirtyIcalEntries.forEach { pushSingleLocalChange(it, calendar) } } + // Deletes attachment blobs that were orphaned by a removed attachment, a hard-deleted entry, or a + // move (source collection). Best-effort: a 2xx or 404 clears the queue entry, anything else is + // left for the next sync to retry. + private suspend fun drainPendingRemoteFileDeletions(calendar: Calendar) { + icalEntryRepository.getPendingRemoteFileDeletions(calendar.id).forEach { pending -> + val status = try { + remote.deleteFile(Url(pending.remoteUrl), credentials) + } catch (e: Exception) { + Napier.d("Remote attachment deletion failed for ${pending.remoteUrl}: ${e.message}") + return@forEach // keep it queued for retry + } + if (status.isSuccess() || status == HttpStatusCode.NotFound) + icalEntryRepository.deletePendingRemoteFileDeletion(pending.id) + } + } + private suspend fun pushSingleLocalChange(dirtyIcalEntry: IcalEntry, calendar: Calendar) { when (dirtyIcalEntry.syncState) { diff --git a/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/data/repository/IcalEntryRepositoryImpl.kt b/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/data/repository/IcalEntryRepositoryImpl.kt index 3a21df76..b429d0ee 100644 --- a/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/data/repository/IcalEntryRepositoryImpl.kt +++ b/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/data/repository/IcalEntryRepositoryImpl.kt @@ -8,10 +8,12 @@ import app.cash.sqldelight.async.coroutines.awaitAsOneOrNull import app.cash.sqldelight.coroutines.asFlow import at.techbee.spectacled.db.SpectacledDatabase import at.techbee.spectacled.screens.core.DatabaseDriverFactory +import at.techbee.spectacled.screens.core.FileManager import at.techbee.spectacled.screens.core.data.ics.IcsDateTime import at.techbee.spectacled.screens.core.domain.Attachment import at.techbee.spectacled.screens.core.domain.AttachmentSyncState import at.techbee.spectacled.screens.core.domain.IcalEntry +import at.techbee.spectacled.screens.core.domain.PendingRemoteFileDeletion import at.techbee.spectacled.screens.core.domain.Status import at.techbee.spectacled.screens.core.domain.SyncState import at.techbee.spectacled.screens.core.domain.repository.IcalEntryRepository @@ -33,7 +35,8 @@ import kotlinx.coroutines.withContext @OptIn(ExperimentalCoroutinesApi::class) class IcalEntryRepositoryImpl( - private val databaseDriverFactory: DatabaseDriverFactory + private val databaseDriverFactory: DatabaseDriverFactory, + private val fileManager: FileManager ) : IcalEntryRepository { private suspend fun getDatabase() = databaseDriverFactory.provideDatabase(SpectacledDatabase.Schema) @@ -269,6 +272,19 @@ class IcalEntryRepositoryImpl( // Re-read fresh so any attachments downloaded just before the move are picked up. val entriesToMove = getIcalEntriesWithSubtasks(icalEntryIds) + // 0. Queue the source-collection blobs for deletion BEFORE re-creating the entries: the + // copy's attachment rows reuse the same UNIQUE uid, so inserting them replaces the + // originals' rows (losing their source remoteUrl). Only enqueue blobs that will actually + // be re-uploaded into the target (attachments with a local copy - matching + // remappedForMove's re-upload branch); a remote-only attachment keeps pointing at its + // source blob after the move, so deleting it would lose the file. + entriesToMove.forEach { entry -> + val orphanedSourceUrls = entry.attachments + .filter { it.localPath != null && !it.remoteUrl.isNullOrBlank() } + .map { it.remoteUrl!! } + enqueueRemoteFileDeletions(entry.calendarId, orphanedSourceUrls) + } + // 1. Re-create each entry in the target calendar. Same uid keeps its identity; clearing // href/etag makes the sync engine PUT it as a new resource into the target collection. entriesToMove.forEach { entry -> @@ -369,9 +385,31 @@ class IcalEntryRepositoryImpl( } } - override suspend fun deleteTrashed(cutoffDateTime: IcsDateTime) { // TODO: Test again! + override suspend fun deleteTrashed(cutoffDateTime: IcsDateTime) { withContext(ioDispatcher) { - getDatabase().icalentry_dtoQueries.deleteTrashed(formatIcsDateTime(cutoffDateTime)?.first) + val db = getDatabase() + val cutoff = formatIcsDateTime(cutoffDateTime)?.first + + // Before hard-deleting the rows (which cascades attachments away), reclaim their + // storage: queue each attachment's server blob for remote deletion and remove its local + // file. A moved entry's original has no attachments here - the copy took over its rows + // via the shared uid - so there is nothing to free for those. + val trashed = db.icalentry_dtoQueries.getTrashedBefore(cutoff).awaitAsList() + if (trashed.isNotEmpty()) { + val calendarIdByEntryId = trashed.associate { it.id to it.calendarId } + val attachments = db.attachment_dtoQueries.getAttachmentsForEntries(trashed.map { it.id }).awaitAsList() + + attachments.forEach { attachment -> + attachment.remoteUrl?.takeIf { it.isNotBlank() }?.let { remoteUrl -> + calendarIdByEntryId[attachment.icalEntryId]?.let { calendarId -> + enqueueRemoteFileDeletions(calendarId, listOf(remoteUrl)) + } + } + attachment.localPath?.let { fileManager.deleteAttachment(it) } + } + } + + db.icalentry_dtoQueries.deleteTrashed(cutoff) } } @@ -414,4 +452,27 @@ class IcalEntryRepositoryImpl( override suspend fun getAttachmentsForEntry(entryId: Long): List = withContext(ioDispatcher) { getDatabase().attachment_dtoQueries.getAttachmentsForEntry(entryId).awaitAsList().map { it.toDomain() } } + + override suspend fun enqueueRemoteFileDeletions(calendarId: Long, remoteUrls: List) { + if (remoteUrls.isEmpty()) return + withContext(ioDispatcher) { + val db = getDatabase() + db.transaction { + remoteUrls.filter { it.isNotBlank() }.forEach { remoteUrl -> + db.pending_remote_deletion_dtoQueries.insertPendingRemoteDeletion(calendarId, remoteUrl) + } + } + } + } + + override suspend fun getPendingRemoteFileDeletions(calendarId: Long): List = withContext(ioDispatcher) { + getDatabase().pending_remote_deletion_dtoQueries.getPendingRemoteDeletionsForCalendar(calendarId).awaitAsList() + .map { PendingRemoteFileDeletion(id = it.id, calendarId = it.calendarId, remoteUrl = it.remoteUrl) } + } + + override suspend fun deletePendingRemoteFileDeletion(id: Long) { + withContext(ioDispatcher) { + getDatabase().pending_remote_deletion_dtoQueries.deletePendingRemoteDeletion(id) + } + } } diff --git a/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/data/webdav/RemoteDataSourceIcalEntry.kt b/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/data/webdav/RemoteDataSourceIcalEntry.kt index 6736336f..2961e768 100644 --- a/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/data/webdav/RemoteDataSourceIcalEntry.kt +++ b/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/data/webdav/RemoteDataSourceIcalEntry.kt @@ -333,3 +333,14 @@ suspend fun downloadFileMultiplatform( } return if (response.status.isSuccess()) response.body() else null // TODO: respond with an actual HttpStatusCode } + +suspend fun deleteFileMultiplatform( + client: HttpClient, + targetUrl: Url, + credentials: Credentials? +): HttpStatusCode { + val response = client.delete(targetUrl) { + credentials?.let { basicAuth(it.username, it.password) } + } + return response.status +} diff --git a/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/data/webdav/WebDavRemoteIcalEntryDataSource.kt b/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/data/webdav/WebDavRemoteIcalEntryDataSource.kt index 1b76cc67..8a3450c7 100644 --- a/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/data/webdav/WebDavRemoteIcalEntryDataSource.kt +++ b/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/data/webdav/WebDavRemoteIcalEntryDataSource.kt @@ -29,6 +29,7 @@ interface WebDavRemoteIcalEntryDataSource { suspend fun deleteResource(calendar: Calendar, icalEntry: IcalEntry, credentials: Credentials?): DeleteResourceResult suspend fun uploadFile(targetUrl: Url, bytes: ByteArray, mimeType: String?, credentials: Credentials?): HttpStatusCode suspend fun downloadFile(sourceUrl: Url, credentials: Credentials?): ByteArray? + suspend fun deleteFile(targetUrl: Url, credentials: Credentials?): HttpStatusCode } class DefaultWebDavRemoteIcalEntryDataSource( @@ -59,4 +60,7 @@ class DefaultWebDavRemoteIcalEntryDataSource( override suspend fun downloadFile(sourceUrl: Url, credentials: Credentials?) = downloadFileMultiplatform(client, sourceUrl, credentials) + + override suspend fun deleteFile(targetUrl: Url, credentials: Credentials?) = + deleteFileMultiplatform(client, targetUrl, credentials) } diff --git a/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/domain/PendingRemoteFileDeletion.kt b/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/domain/PendingRemoteFileDeletion.kt new file mode 100644 index 00000000..4eb2caa9 --- /dev/null +++ b/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/domain/PendingRemoteFileDeletion.kt @@ -0,0 +1,11 @@ +package at.techbee.spectacled.screens.core.domain + +/** + * A remote attachment blob that is no longer referenced by any entry and still needs to be deleted + * from its CalDAV collection. Queued locally and drained by the sync engine. + */ +data class PendingRemoteFileDeletion( + val id: Long, + val calendarId: Long, + val remoteUrl: String +) diff --git a/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/domain/repository/IcalEntryRepository.kt b/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/domain/repository/IcalEntryRepository.kt index bb2d2d0c..0c304004 100644 --- a/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/domain/repository/IcalEntryRepository.kt +++ b/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/core/domain/repository/IcalEntryRepository.kt @@ -4,6 +4,7 @@ import androidx.compose.ui.graphics.Color import at.techbee.spectacled.screens.core.data.ics.IcsDateTime import at.techbee.spectacled.screens.core.domain.Attachment import at.techbee.spectacled.screens.core.domain.IcalEntry +import at.techbee.spectacled.screens.core.domain.PendingRemoteFileDeletion import at.techbee.spectacled.screens.core.domain.Status import at.techbee.spectacled.screens.core.domain.SyncState import io.ktor.http.Url @@ -56,4 +57,10 @@ interface IcalEntryRepository { suspend fun insertOrUpdateAttachment(attachment: Attachment) suspend fun deleteAttachment(id: Long) suspend fun getAttachmentsForEntry(entryId: Long): List + + // Remote attachment-blob cleanup queue + /** Queues attachment blob [remoteUrls] (in [calendarId]'s collection) for later deletion by sync. */ + suspend fun enqueueRemoteFileDeletions(calendarId: Long, remoteUrls: List) + suspend fun getPendingRemoteFileDeletions(calendarId: Long): List + suspend fun deletePendingRemoteFileDeletion(id: Long) } diff --git a/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/details/presentation/DetailsViewModel.kt b/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/details/presentation/DetailsViewModel.kt index cdb2cf6b..84d52596 100644 --- a/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/details/presentation/DetailsViewModel.kt +++ b/shared/src/commonMain/kotlin/at/techbee/spectacled/screens/details/presentation/DetailsViewModel.kt @@ -857,7 +857,13 @@ class DetailsViewModel( val attachment = _state.value.icalEntry.attachments.find { it.uid == attachmentUid } if (attachment != null) { attachment.localPath?.let { fileManager.deleteAttachment(it) } - + + // If it was already on the server, queue its blob for deletion; re-saving the entry + // only drops the ATTACH reference, it never removes the uploaded file. + attachment.remoteUrl?.takeIf { it.isNotBlank() }?.let { remoteUrl -> + icalEntryRepository.enqueueRemoteFileDeletions(_state.value.icalEntry.calendarId, listOf(remoteUrl)) + } + _state.update { it.copy( icalEntry = it.icalEntry.copy( diff --git a/shared/src/commonMain/sqldelight/at/techbee/spectacled/sqldelight/13.sqm b/shared/src/commonMain/sqldelight/at/techbee/spectacled/sqldelight/13.sqm new file mode 100644 index 00000000..db67d4e6 --- /dev/null +++ b/shared/src/commonMain/sqldelight/at/techbee/spectacled/sqldelight/13.sqm @@ -0,0 +1,7 @@ +CREATE TABLE PendingRemoteDeletionDto ( + id INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, + calendarId INTEGER NOT NULL, + remoteUrl TEXT NOT NULL UNIQUE +); + +CREATE INDEX idx_pending_remote_deletion_calendarId ON PendingRemoteDeletionDto(calendarId); diff --git a/shared/src/commonMain/sqldelight/at/techbee/spectacled/sqldelight/icalentry_dto.sq b/shared/src/commonMain/sqldelight/at/techbee/spectacled/sqldelight/icalentry_dto.sq index 7c801424..fbf74e04 100644 --- a/shared/src/commonMain/sqldelight/at/techbee/spectacled/sqldelight/icalentry_dto.sq +++ b/shared/src/commonMain/sqldelight/at/techbee/spectacled/sqldelight/icalentry_dto.sq @@ -165,6 +165,9 @@ SET url = :url WHERE calendarId = :calendarId AND uid = :uid; +getTrashedBefore: +SELECT id, calendarId FROM IcalEntryDto WHERE lastModified < :cutOffTimestamp AND syncState = 'REMOTE_DELETED_LOCAL_TRASHBIN'; + deleteTrashed: DELETE FROM IcalEntryDto WHERE lastModified < :cutOffTimestamp AND syncState = 'REMOTE_DELETED_LOCAL_TRASHBIN'; diff --git a/shared/src/commonMain/sqldelight/at/techbee/spectacled/sqldelight/pending_remote_deletion_dto.sq b/shared/src/commonMain/sqldelight/at/techbee/spectacled/sqldelight/pending_remote_deletion_dto.sq new file mode 100644 index 00000000..019959cb --- /dev/null +++ b/shared/src/commonMain/sqldelight/at/techbee/spectacled/sqldelight/pending_remote_deletion_dto.sq @@ -0,0 +1,21 @@ +-- Queue of remote attachment blobs that are no longer referenced by any entry and must be +-- deleted from their CalDAV collection. Deliberately NOT a child of IcalEntryDto: entry deletes +-- cascade to AttachmentDto, so the blob URL has to survive independently of the entry row. +-- Drained by the sync engine per calendar (best-effort, retried until the server confirms). +CREATE TABLE PendingRemoteDeletionDto ( + id INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, + calendarId INTEGER NOT NULL, + remoteUrl TEXT NOT NULL UNIQUE +); + +CREATE INDEX idx_pending_remote_deletion_calendarId ON PendingRemoteDeletionDto(calendarId); + +insertPendingRemoteDeletion: +INSERT OR IGNORE INTO PendingRemoteDeletionDto (calendarId, remoteUrl) +VALUES (?, ?); + +getPendingRemoteDeletionsForCalendar: +SELECT * FROM PendingRemoteDeletionDto WHERE calendarId = ?; + +deletePendingRemoteDeletion: +DELETE FROM PendingRemoteDeletionDto WHERE id = ?; diff --git a/shared/src/commonTest/kotlin/at/techbee/spectacled/screens/core/SyncCoordinatorTest.kt b/shared/src/commonTest/kotlin/at/techbee/spectacled/screens/core/SyncCoordinatorTest.kt index 065569a9..71eaac96 100644 --- a/shared/src/commonTest/kotlin/at/techbee/spectacled/screens/core/SyncCoordinatorTest.kt +++ b/shared/src/commonTest/kotlin/at/techbee/spectacled/screens/core/SyncCoordinatorTest.kt @@ -17,6 +17,7 @@ import at.techbee.spectacled.screens.core.domain.CalendarSyncStatus import at.techbee.spectacled.screens.core.domain.CalendarSyncStatusType import at.techbee.spectacled.screens.core.domain.HomeCollection import at.techbee.spectacled.screens.core.domain.IcalEntry +import at.techbee.spectacled.screens.core.domain.PendingRemoteFileDeletion import at.techbee.spectacled.screens.core.domain.Principal import at.techbee.spectacled.screens.core.domain.Status import at.techbee.spectacled.screens.core.domain.SyncState @@ -378,6 +379,7 @@ private class FakeRemote( override suspend fun uploadFile(targetUrl: Url, bytes: ByteArray, mimeType: String?, credentials: Credentials?) = HttpStatusCode.Created override suspend fun downloadFile(sourceUrl: Url, credentials: Credentials?): ByteArray = throw AssertionError("downloadFile not expected in a sync test") + override suspend fun deleteFile(targetUrl: Url, credentials: Credentials?) = HttpStatusCode.NoContent } private data class SyncMetadataUpdate(val etag: String?, val href: Url?, val syncState: SyncState?, val id: Long) @@ -432,6 +434,11 @@ private class FakeIcalEntryRepository( override suspend fun deleteTrashed(cutoffDateTime: IcsDateTime) = TODO() override suspend fun deleteAttachment(id: Long) = TODO() override suspend fun getAttachmentsForEntry(entryId: Long): List = TODO() + override suspend fun enqueueRemoteFileDeletions(calendarId: Long, remoteUrls: List) = TODO() + override suspend fun deletePendingRemoteFileDeletion(id: Long) = TODO() + + // Drained on every sync - return nothing so the sync path issues no remote deletions. + override suspend fun getPendingRemoteFileDeletions(calendarId: Long): List = emptyList() } private class FakeCalendarRepository : CalendarRepository {