From 313b086effa3b1e4203597b8843414e8baef408d Mon Sep 17 00:00:00 2001 From: SiteRelEnby <125829806+SiteRelEnby@users.noreply.github.com> Date: Fri, 23 Oct 2026 23:55:42 -0400 Subject: [PATCH] fix(files): ask the server which uploads are unused "Delete unused files" decided that itself, from the system avatar and member avatars and nothing else. Banners, images embedded in bios and journal entries, and anything revision history still holds were all reported as unused, and the button then deleted them. The server does no reference check of its own on a delete, so nothing downstream caught it. Reproduced on an account whose only two uploads were a member avatar and the live system banner: the old build offered to permanently delete the banner, while the server's own dry run reported no orphans at all. It now asks `POST /v1/files/cleanup/dry-run`, which checks every place a file can be referenced, and matches the result back to the listing by storage key rather than by serve URL, since the URL depends on how the instance hands files out. Deliberately the dry run rather than the real cleanup: that one deletes server-side in one shot and skips System Safety's image category, so deleting the files one at a time from here keeps the safeguard this client already honoured. --- CHANGELOG.md | 13 +++ .../lupine/sheaf/data/api/SheafApiService.kt | 16 ++++ .../systems/lupine/sheaf/data/model/Models.kt | 19 ++++ .../sheaf/ui/settings/SettingsViewModel.kt | 31 +++++-- .../sheaf/ui/settings/OrphanFilesTest.kt | 90 +++++++++++++++++++ 5 files changed, 161 insertions(+), 8 deletions(-) create mode 100644 sheaf/app/src/test/java/systems/lupine/sheaf/ui/settings/OrphanFilesTest.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index cc9177a..9856ea0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,19 @@ All notable changes to the Sheaf Android client are recorded here. Format loosel follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); the project uses semantic versioning (`MAJOR.MINOR.PATCH`). +## [Unreleased] + +### Fixed + +- **"Delete unused files" no longer deletes files that are in use.** It worked + out which uploads were unused by looking at your system avatar and your + members' avatars, and nothing else, so banners, any image inside a bio or a + journal entry, and anything your edit history still holds were all listed as + unused and deleted on confirmation. The app now asks the server, which checks + all of those places, and still deletes one file at a time so System Safety's + image safeguard applies. If you lost an image this way it cannot be recovered + from the app. + ## [1.6.0] - 2026-10-23 ### Added diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/SheafApiService.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/SheafApiService.kt index 41be0eb..a3dedc8 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/SheafApiService.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/SheafApiService.kt @@ -535,6 +535,22 @@ interface SheafApiService { @GET("/v1/files/list") suspend fun listFiles(): List + /** + * Which files the server considers unused, without deleting anything. + * + * Asked rather than worked out locally: the server checks avatars, banners, + * bios, journal entries and revision history, and a client that reimplements + * that list gets it wrong the moment the server learns a new place to + * reference a file. + * + * Deliberately the dry run rather than `POST /v1/files/cleanup`. The real + * cleanup deletes server-side in one shot and skips System Safety's image + * category; deleting the files one by one from here keeps that safeguard, + * which is the one thing this client was already doing right. + */ + @POST("/v1/files/cleanup/dry-run") + suspend fun previewFileCleanup(): FileCleanupPreview + @HTTP(method = "DELETE", path = "/v1/files/{id}", hasBody = true) suspend fun deleteFile( @Path("id") id: String, diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/data/model/Models.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/data/model/Models.kt index baefb7d..c316e0c 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/data/model/Models.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/data/model/Models.kt @@ -992,6 +992,25 @@ data class FileUsage( @Json(name = "file_count") val fileCount: Int, ) +/** + * What `POST /v1/files/cleanup/dry-run` would delete. + * + * The server owns this question. Working it out on the client means knowing + * every place a file can be referenced, and this app knew about two of them: + * it counted avatars and nothing else, so banners, every image embedded in a + * bio or a journal entry, and everything held by revision history came back as + * unused. + * + * [keys] is absent when nothing is orphaned, hence the default. + */ +@JsonClass(generateAdapter = true) +data class FileCleanupPreview( + val orphaned: Int = 0, + @Json(name = "freed_bytes") val freedBytes: Long = 0, + @Json(name = "dry_run") val dryRun: Boolean = true, + val keys: List = emptyList(), +) + @JsonClass(generateAdapter = true) data class FileRead( val id: String, diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsViewModel.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsViewModel.kt index 102e0b9..c9822d1 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsViewModel.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsViewModel.kt @@ -80,6 +80,23 @@ data class SettingsUiState( private const val CLIENT_ID = "android" +/** + * The listed files the server named as unused. + * + * Matched on [FileRead.key], which is what the server reports, and not on the + * serve URL: the URL depends on how the instance hands files out, so a CDN or + * signed-URL instance would match nothing and quietly report that everything + * is unused. + * + * The rows come from the listing rather than being built from the keys so each + * keeps its size and date, and so it can be deleted through the per-file path + * that honours System Safety. + */ +internal fun orphanFiles(files: List, orphanKeys: List): List { + val keys = orphanKeys.toSet() + return files.filter { it.key in keys } +} + @HiltViewModel class SettingsViewModel @Inject constructor( private val api: SheafApiService, @@ -508,14 +525,12 @@ class SettingsViewModel @Inject constructor( _state.update { it.copy(isCheckingFiles = true, fileError = null, orphanedFiles = null) } viewModelScope.launch { runCatching { - val files = api.listFiles() - val members = api.listMembers() - val system = api.getOwnSystem() - val usedUrls = buildSet { - system.avatarUrl?.let { add(it) } - members.forEach { m -> m.avatarUrl?.let { add(it) } } - } - files.filter { it.url !in usedUrls } + // The server decides what is unused. This used to be worked out + // here from the system avatar and member avatars alone, which + // meant banners, images embedded in bios and journal entries, + // and anything revision history still holds were all reported + // as unused, and the button then deleted them. + orphanFiles(api.listFiles(), api.previewFileCleanup().keys) }.onSuccess { orphans -> _state.update { it.copy(isCheckingFiles = false, orphanedFiles = orphans) } }.onFailure { e -> diff --git a/sheaf/app/src/test/java/systems/lupine/sheaf/ui/settings/OrphanFilesTest.kt b/sheaf/app/src/test/java/systems/lupine/sheaf/ui/settings/OrphanFilesTest.kt new file mode 100644 index 0000000..636f07b --- /dev/null +++ b/sheaf/app/src/test/java/systems/lupine/sheaf/ui/settings/OrphanFilesTest.kt @@ -0,0 +1,90 @@ +package systems.lupine.sheaf.ui.settings + +import systems.lupine.sheaf.data.model.FileCleanupPreview +import systems.lupine.sheaf.data.model.FileRead +import systems.lupine.sheaf.di.NetworkModule +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +/** + * Which files "Delete unused files" offers up. + * + * This used to be decided here, from the system avatar and member avatars and + * nothing else, so a banner, an image embedded in a bio or a journal entry, and + * anything revision history still held all came back as unused. The button then + * deleted them, and the server does no reference check of its own on a delete. + * + * The answer now comes from the server, which checks all of those places. These + * tests cover the matching, which is the part still done here, and the two + * response shapes the preview comes back as. + */ +class OrphanFilesTest { + + private fun file(id: String, key: String, url: String = "/v1/files/$key") = FileRead( + id = id, + key = key, + url = url, + purpose = "avatar", + contentType = "image/png", + sizeBytes = 1, + createdAt = "2026-01-01T00:00:00Z", + ) + + @Test fun `only the files the server named come back`() { + val files = listOf( + file("1", "avatars/sys/a.png"), + file("2", "banners/sys/b.png"), + file("3", "bios/sys/c.png"), + ) + val orphans = orphanFiles(files, listOf("bios/sys/c.png")) + assertEquals(listOf("3"), orphans.map { it.id }) + } + + @Test fun `a banner the system is using is not offered for deletion`() { + // The case found in the field: the account's only two files were a + // member avatar and the live system banner, and the old rule flagged + // the banner. The server reports no orphans at all here. + val files = listOf(file("1", "avatars/sys/a.png"), file("2", "banners/sys/b.png")) + assertTrue(orphanFiles(files, emptyList()).isEmpty()) + } + + @Test fun `matching is on the key, not the serve URL`() { + // The old rule compared serve URLs. How a file is served depends on the + // instance: a CDN or a signed URL means the listing's url does not look + // like anything the server reported, so url matching would come back + // with either nothing or everything. + val files = listOf( + file("1", "avatars/sys/a.png", url = "https://cdn.example.net/avatars/sys/a.png?token=x"), + ) + assertEquals(listOf("1"), orphanFiles(files, listOf("avatars/sys/a.png")).map { it.id }) + } + + @Test fun `a key the listing does not have is ignored`() { + // The two calls are not atomic, so the preview can name a file that has + // since gone. Nothing to delete rather than an invented row. + assertTrue(orphanFiles(listOf(file("1", "a.png")), listOf("gone.png")).isEmpty()) + } + + @Test fun `the no-orphans response has no keys at all`() { + // The server omits `keys` entirely when nothing is orphaned, so this + // has to decode to an empty list rather than failing or reading null. + val preview = NetworkModule.provideMoshi() + .adapter(FileCleanupPreview::class.java) + .fromJson("""{"orphaned":0,"freed_bytes":0,"dry_run":true}""")!! + assertEquals(0, preview.orphaned) + assertTrue(preview.keys.isEmpty()) + } + + @Test fun `a preview with orphans carries their keys`() { + val preview = NetworkModule.provideMoshi() + .adapter(FileCleanupPreview::class.java) + .fromJson( + """{"orphaned":2,"freed_bytes":2048,"dry_run":true,""" + + """"keys":["bios/sys/c.png","journals/sys/d.png"]}""", + )!! + assertEquals(2, preview.orphaned) + assertEquals(2048L, preview.freedBytes) + assertEquals(listOf("bios/sys/c.png", "journals/sys/d.png"), preview.keys) + } +}