diff --git a/docs/temp-file-cleanup-analysis.md b/docs/temp-file-cleanup-analysis.md index 25299b935b..d9acd1ea55 100644 --- a/docs/temp-file-cleanup-analysis.md +++ b/docs/temp-file-cleanup-analysis.md @@ -8,18 +8,19 @@ as soon as they are no longer needed rather than deferring to cache cleanup. ## Temporary File Creation Sites (Android) -| Area | File | Creates | Cleanup | Notes | -|------|------|---------|---------|-------| -| Image compression | `MediaCompressor.kt:94` | Temp copy via `MediaCompressorFileUtils.from()` | UploadOrchestrator `finally` | Deferred to cache cleanup | -| Image metadata strip | `MetadataStripper.kt:199` | `stripped_*.jpg` in cacheDir | UploadOrchestrator `finally` | Deferred to cache cleanup | -| Video metadata strip | `MetadataStripper.kt:238` | `stripped_video_*.mp4` in cacheDir | UploadOrchestrator `finally` | Deferred to cache cleanup | -| Audio metadata strip | `MetadataStripper.kt:286` | `stripped_audio_*.m4a` in cacheDir | UploadOrchestrator `finally` | Deferred to cache cleanup | -| MP3 metadata strip | `MetadataStripper.kt:307,363` | `mp3_input_*` + `stripped_mp3_*` | Mixed: some immediate, some orchestrator | MP3 input file cleaned inline | -| File encryption | `EncryptFiles.kt:47` | `EncryptFiles*.encrypted` in cacheDir | UploadOrchestrator `finally` | Deferred to cache cleanup | -| URI temp copy | `MediaCompressorFileUtils.kt:41` | Random UUID temp file | Caller-dependent | No self-cleanup | -| Voice anonymization | `VoiceAnonymizationController.kt:85` | Distorted voice files | `deleteDistortedFiles()` explicit | Cleaned by controller | -| Video sharing | `ZoomableContentView.kt:905` | Temp video + sharable copy | Delayed GlobalScope (1 min) | Intentional delay for sharing | -| Camera capture | `TakePicture.kt:244` | Camera temp file | System/caller | Managed by camera provider | +| Area | File | Creates | Cleanup | Status | +|------|------|---------|---------|--------| +| Image compression | `MediaCompressor.kt:94` | Temp copy via `MediaCompressorFileUtils.from()` | Deleted immediately after compression | FIXED | +| Image metadata strip | `MetadataStripper.kt:199` | `stripped_*.jpg` in cacheDir | Deleted eagerly by orchestrator | FIXED | +| Video metadata strip | `MetadataStripper.kt:238` | `stripped_video_*.mp4` in cacheDir | Deleted eagerly by orchestrator | FIXED | +| Audio metadata strip | `MetadataStripper.kt:286` | `stripped_audio_*.m4a` in cacheDir | Deleted eagerly by orchestrator | FIXED | +| MP3 metadata strip | `MetadataStripper.kt:307,363` | `mp3_input_*` + `stripped_mp3_*` | Mixed: some immediate, some orchestrator | Already OK | +| File encryption | `EncryptFiles.kt:47` | `EncryptFiles*.encrypted` in cacheDir | Deleted eagerly by orchestrator | FIXED | +| URI temp copy | `MediaCompressorFileUtils.kt:41` | Random UUID temp file | Deleted by MediaCompressor after use | FIXED | +| Video compression | `VideoCompressionHelper.kt` | Compressed video in app storage | Abandoned file deleted when larger than original | FIXED | +| Voice anonymization | `VoiceAnonymizationController.kt:85` | Distorted voice files | `deleteDistortedFiles()` explicit | Already OK | +| Video sharing | `ZoomableContentView.kt:905` | Temp video + sharable copy | Delayed GlobalScope (1 min) | Skipped (intentional) | +| Camera capture | `TakePicture.kt:244` | Camera temp file | System/caller | Skipped (system-managed) | ## Temporary File Creation Sites (Desktop) @@ -30,80 +31,171 @@ as soon as they are no longer needed rather than deferring to cache cleanup. ## The Upload Pipeline -The `UploadOrchestrator` is the central cleanup coordinator. Its `finally` blocks call -`deleteTempUri()` to delete intermediate files after upload completes. The pipeline stages: +The `UploadOrchestrator` is the central cleanup coordinator. Each intermediate temp file +is now deleted as soon as the next pipeline stage produces its output: ``` -1. MediaCompressorFileUtils.from() --> temp copy of original URI -2. MediaCompressor.compress() --> compressed file (may replace #1) -3. MetadataStripper.strip*() --> stripped file (new temp) -4. (optional) EncryptFiles.encrypt() --> encrypted file (new temp) +1. MediaCompressorFileUtils.from() --> temp copy of original URI +2. MediaCompressor.compress() --> compressed file; temp copy from #1 deleted immediately +3. MetadataStripper.strip*() --> stripped file; compressed file from #2 deleted immediately +4. (optional) EncryptFiles.encrypt() --> encrypted file; stripped file from #3 deleted immediately 5. Upload to server -6. finally: delete all intermediates via deleteTempUri() +6. finally: delete the last remaining intermediate ``` -Each stage may produce a new temp file. Currently, all intermediates accumulate and are -only cleaned up in the `finally` block after the upload succeeds or fails. +## What Was Fixed -## Cleanup Patterns in Use +### 1. MediaCompressor temp file leak (MediaCompressor.kt) +- `MediaCompressorFileUtils.from()` created a temp copy that was never deleted +- Now deleted immediately after `Compressor.compress()` produces a separate output file +- Also cleaned up on compression failure (catch block) -### 1. Chained Deletion (UploadOrchestrator) -- Most common pattern -- `finally` blocks at `L374-379` (upload) and `L410-415` (uploadEncrypted) -- Deletes all intermediate temp files after pipeline completes -- Checks `tempUri != originalUri` to avoid deleting user's original file +### 2. Eager pipeline cleanup (UploadOrchestrator.kt) +- `upload()`: compressed file deleted right after stripping produces `finalUri` +- `uploadEncrypted()`: compressed file deleted after stripping, stripped file deleted + after encryption — only the encrypted file survives until after upload +- Cancel path also cleans up compressed file via `.also {}` block +- `finally` block now only handles the last surviving intermediate -### 2. Delayed Deletion (Video Sharing) -- `ZoomableContentView.kt:953-960` -- Uses `GlobalScope.launch(Dispatchers.IO)` with 1 minute delay -- Intentional: allows recipient app time to read the shared file -- **Exception: this delay is necessary and should not be shortened** +### 3. Abandoned compressed video (VideoCompressionHelper.kt) +- When compressed video is larger than original, the original is used instead +- The abandoned compressed file was leaked — now deleted before returning -### 3. Explicit Cleanup Calls (Voice) -- `VoiceAnonymizationController.deleteDistortedFiles()` at `L108-123` -- Called explicitly by `ShortNotePostViewModel` at `L1336` -- Good pattern: controller owns its temp files +## What Was Skipped -### 4. deleteOnExit() (Desktop) -- Used by `ClipboardPasteHandler` and `DesktopMediaCompressor` -- Files persist until JVM process exits -- Not ideal for long-running desktop app +### Desktop temp files (out of scope for this change) +- `ClipboardPasteHandler.kt` and `DesktopMediaCompressor.kt` use `deleteOnExit()` +- Files persist until JVM process exits — not ideal for a long-running desktop app +- `DesktopUploadOrchestrator.kt` uses bare `processedFile.delete()` with no error + handling or logging, diverging from the Android `deleteTempUri` pattern +- **Reason:** User requested Android-only focus for this iteration -## Opportunities for More Aggressive Cleanup +### Voice anonymization intermediates +- `VoiceAnonymizationController.deleteDistortedFiles()` is already reasonably aggressive +- Called explicitly by `ShortNotePostViewModel` after upload +- **Reason:** Already working well, no leak identified -### 1. Pipeline Stage Cleanup (UploadOrchestrator) +### Video sharing delay +- `ZoomableContentView.kt` uses a 1-minute delay before cleanup +- **Reason:** Intentional — receiving app needs time to read the shared file -**Current:** All intermediates deleted in `finally` after upload. - -**Proposed:** Delete each intermediate as soon as the next stage produces output. - -Example: after `MetadataStripper` produces a stripped file, delete the compressed file -from the previous stage immediately, rather than keeping it alive through upload + finally. - -### 2. Desktop Temp Files - -**Current:** `deleteOnExit()` — files persist for entire app session. - -**Proposed:** Delete immediately after upload completes, matching the Android -`UploadOrchestrator` pattern via `DesktopUploadOrchestrator`. - -### 3. MediaCompressorFileUtils Intermediate - -**Current:** `from()` creates a temp copy at `L41`. If compression produces a *different* -file, the original temp copy is orphaned until `UploadOrchestrator.finally`. - -**Proposed:** `MediaCompressor` should delete the `from()` temp file immediately after -compression succeeds (when the compressed file differs from the input). - -### 4. Voice Anonymization Intermediates - -**Current:** `deleteDistortedFiles()` called after upload. - -**Proposed:** Already reasonably aggressive. Could delete each intermediate distortion -result as soon as the next processing step completes, if there are multiple stages. +### Camera capture temp files +- `TakePicture.kt` creates temp files via camera provider +- **Reason:** Managed by the Android system/camera provider, not our responsibility ## Exception: Video Sharing Sharing a video to other Android apps requires the temporary file to remain accessible for at least 1 minute. The current `SHARED_VIDEO_CLEANUP_DELAY_MS` delay in -`ZoomableContentView.kt` handles this correctly and should **not** be made more aggressive. \ No newline at end of file +`ZoomableContentView.kt` handles this correctly and should **not** be made more aggressive. + +## Manual Test Plan + +### Setup + +Enable `adb logcat` filtering to observe cleanup behavior: + +```bash +adb logcat -s MediaCompressor:* UploadOrchestrator:* VideoCompressionHelper:* MetadataStripper:* +``` + +To verify temp files are actually being deleted, monitor the cache directory before and +after each test: + +```bash +adb shell "ls -la /data/data/com.vitorpamplona.amethyst/cache/ | grep -E 'stripped_|EncryptFiles|mp3_input|stripped_mp3|stripped_video|stripped_audio'" +``` + +### Test 1: Image upload with compression + +1. Open a new note compose screen +2. Attach a JPEG photo from the gallery +3. Set compression quality to Medium +4. Post the note +5. **Verify in logcat:** + - `MediaCompressor: Image compression success` appears + - `MediaCompressor: Failed to delete temp file` does NOT appear + - `UploadOrchestrator: Deleted temp file` appears (for the stripped file after upload) +6. **Verify in cache dir:** No `stripped_*.jpg` files remain after upload completes + +### Test 2: Image upload without compression + +1. Open a new note compose screen +2. Attach a JPEG photo from the gallery +3. Set compression quality to Uncompressed +4. Post the note +5. **Verify in logcat:** + - No `MediaCompressor` compression log appears + - `UploadOrchestrator: Deleted temp file` appears for the stripped file +6. **Verify in cache dir:** No `stripped_*` files remain + +### Test 3: Video upload with compression + +1. Open a new note compose screen +2. Attach a video from the gallery +3. Set compression quality to Medium +4. Post the note +5. **Verify in logcat:** + - `VideoCompressionHelper: Compression success` appears + - `UploadOrchestrator: Deleted temp file` appears +6. **Verify in cache dir:** No `stripped_video_*.mp4` files remain + +### Test 4: Video compression produces larger file + +1. Attach a very small or already-compressed video +2. Set compression to Low quality +3. Post the note +4. **Verify in logcat:** + - `VideoCompressionHelper: Compressed file larger than original. Using original.` appears + - The compressed file is deleted (no orphaned file in cache) + +### Test 5: Audio/voice message upload + +1. Record a voice message in a note or reply +2. Send it +3. **Verify in logcat:** + - `UploadOrchestrator: Deleted temp file` appears for the stripped audio +4. **Verify in cache dir:** No `stripped_audio_*.m4a` files remain + +### Test 6: MP3 upload + +1. Attach an MP3 file (with ID3 tags) from the file picker +2. Post the note +3. **Verify in logcat:** + - `MetadataStripper: Stripped ID3 tags from MP3` appears + - `UploadOrchestrator: Deleted temp file` appears +4. **Verify in cache dir:** No `mp3_input_*` or `stripped_mp3_*` files remain + +### Test 7: Encrypted file upload (NIP-44 DM) + +1. Open a DM conversation +2. Attach an image +3. Send the message (triggers encrypted upload path) +4. **Verify in logcat:** + - Compressed file is deleted after stripping + - Stripped file is deleted after encryption + - Encrypted file is deleted after upload + - Three separate `UploadOrchestrator: Deleted temp file` log lines appear +5. **Verify in cache dir:** No `stripped_*` or `EncryptFiles*` files remain + +### Test 8: Upload cancellation + +1. Open a new note compose screen +2. Attach a large image or video +3. Cancel the upload while compression or upload is in progress +4. **Verify in cache dir:** No temp files remain from the cancelled upload + +### Test 9: Video sharing to other apps + +1. Open a note with a video +2. Long-press or use the share button to share the video to another app +3. **Verify:** The receiving app successfully receives the video +4. **Verify:** After ~1 minute, the temp file in the share directory is cleaned up +5. **This test confirms the 1-minute delay was not broken by our changes** + +### Test 10: Image compression failure fallback + +1. Attach a GIF or SVG file (compression is skipped for these) +2. Post the note +3. **Verify:** Upload succeeds using the original file +4. **Verify in cache dir:** No orphaned temp files \ No newline at end of file