Fix/orphaned crypted file cleanup#15098
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughStartup routines added to CLI, desktop and mobile to remove orphaned Changes
Sequence Diagram(s)sequenceDiagram
participant Startup as Startup Manager
participant App as Application (CLI/Desktop/Mobile)
participant Resource as Resource
participant FS as fsDriver
participant Logger as Logger
Startup->>App: begin startup sequence
App->>Resource: emptyTempEncryptionCache()
Resource->>FS: stat/remove tempDir/encryptionCache/*.crypted
FS-->>Resource: success / error
Resource->>Logger: warn(...) on error
Resource-->>App: return (no throw)
App-->>Startup: continue startup (commands/GUI)
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/lib/models/Resource.ts (1)
269-271: Log the error details for debuggability.The caught error is not included in the log message, making it harder to diagnose cleanup failures. Additionally, using
infolevel for failures is inconsistent with the calling code in mobile/CLI which useswarn.♻️ Suggested improvement
} catch (error) { - this.logger().info('Could not clear temporary encryption cache.'); + this.logger().warn('Could not clear temporary encryption cache:', error); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/models/Resource.ts` around lines 269 - 271, The catch block in Resource.ts that currently does this.logger().info('Could not clear temporary encryption cache.') should be changed to log the caught error details and use a warning level: replace the info call with this.logger().warn(...) and include the error object (e.g., this.logger().warn('Could not clear temporary encryption cache', { error })) so the error message and stack are captured for debugging; ensure the caught variable (error) is preserved and passed to the logger rather than discarded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/lib/models/Resource.ts`:
- Around line 269-271: The catch block in Resource.ts that currently does
this.logger().info('Could not clear temporary encryption cache.') should be
changed to log the caught error details and use a warning level: replace the
info call with this.logger().warn(...) and include the error object (e.g.,
this.logger().warn('Could not clear temporary encryption cache', { error })) so
the error message and stack are captured for debugging; ensure the caught
variable (error) is preserved and passed to the logger rather than discarded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d8dc375-03d1-4886-b623-b663044e24db
📒 Files selected for processing (5)
packages/app-cli/app/app.tspackages/app-desktop/app.tspackages/app-mobile/utils/buildStartupTasks.tspackages/lib/models/Resource.test.tspackages/lib/models/Resource.ts
|
|
||
| addTask('app/updateTray', () => this.updateTray()); | ||
|
|
||
| addTask('app/deleteOrphanedTempCache', async () => await Resource.emptyTempEncryptionCache()); |
There was a problem hiding this comment.
Any reason why there isn't a try catch for this call?
There was a problem hiding this comment.
Thanks for review!
emptyTempEncryptionCache itself handels the error so basically it never will throw an error, though I was particularly not sure about any edge cases arising in mobile or cli so I added the double try catch there.
Now that you've mentioned it, I think it would be better to add a try catch here too to maintain the consistency.
I will just make a quick commit by adding the nitpick and try catch
|
Is it possible when starting the app, that the sync could be in progress while the emptying of the temp folder occurs, and this could delete a crypted file while it is uploading? |
I wanted to be 100% sure, so I verified this by forcing an artificial delay on the sweep and seeing the startup logs. There's no risk of a race condition here. The logs confirm that the startup sweep finishes during the initial boot sequence (within the first ~2 seconds), while the background Synchronizer doesn't even wake up to index resources until about 11 seconds later. Because their lifecycles are completely isolated + Even if an unexpected overlap occurred, the |
If you put some random 20gb file in the tempCryptedPath, would it still delete within the first 2 seconds? |
For a single large file, deletion is usually very fast since the OS primarily removes metadata, so in practice it should still complete within the startup window. That said, I wouldn’t rely on strict timing guarantees. Even with a larger number of files (e.g., up to ~10k in The main safety comes from the blocking startup sequence (cleanup runs before the Synchronizer starts and the app waits for it to complete), so even in unlikely edge cases (e.g., temporary buildup of files), we avoid impacting active uploads. |
laurent22
left a comment
laurent22
left a comment
There was a problem hiding this comment.
Thanks for looking into this. I think that's a sensible approach and indeed should be much faster than scanning the resource folder.
I looked at the benchmark and maybe I'm lacking context, but was that done on the CLI app? And if so would it be possible to try on the Android app?
We need to think about the worst case - some people import hundreds of thousands of notes and will have as many .crypted files. If you have, say, a million .crypted files in that folder in Android, how does it perform?
Als I'm not a fan of calling this "temp cache". Maybe just "encryptionCache"?
|
|
||
| if (await this.fsDriver().exists(tempCacheDir)) { | ||
| try { | ||
| await this.fsDriver().remove(tempCacheDir); |
There was a problem hiding this comment.
Before proceeding please add a message "Clearing encryption cache..." too
To give some context on the benchmark, I actually didn't run it in the CLI app. I wrote a quick standalone Node script just to test the raw disk I/O difference between scanning a huge folder vs. dropping an isolated one. As for the 1 million files on Android scenario: the app shouldn't ever hold that many at once since the sync engine processes uploads in small batches. But even if a massive backlog did pile up over time, dropping the directory natively on Android is definitely the safest bet. It bypasses the React Native JS bridge entirely. Here is the comparison benchmark for mobile too : 
Here is the deletion time for 1 million file :- |
coderabbitai
Bot
removed
bug
3 participants
Problem
Fixes #9093
When End-to-End Encryption (E2EE) is enabled, Joplin creates temporary
.cryptedfiles during the resource upload process. If an upload is interrupted or the application crashes, these files remain orphaned in the resources directory.Previously, cleaning these files required scanning the entire
resourcesfolder on startup. For users with large libraries (10,000+ items), this approach is inefficient as startup time scales linearly with the number of resources. This results in unnecessary disk clutter and potential performance degradation.Solution
This PR introduces an$O(1)$ cleanup solution by isolating temporary encryption files.
here is the benchmark testing :
https://discourse.joplinapp.org/t/gsoc-2026-local-note-encryption-draft-proposal-and-poc/49012/33?u=akshajrawat
Key Changes:
Isolation: Temporary
.cryptedfiles are now redirected to a dedicated subdirectory:[profile]/tmp/temp_cache.Efficient Cleanup: Added a startup hook across Desktop, CLI, and Mobile platforms to wipe and recreate the
temp_cachedirectory upon boot.Redirection: Updated
Resource.fullPathForSyncUploadto utilize this new path logic.Maintenance: Standard post-decryption cleanup continues to function in the main directory to ensure no regression in existing cleanup logic.
This change is low-risk as it only affects temporary files used during the sync-upload lifecycle and does not modify primary resource storage logic.
Test Plan
packages/lib/models/Resource.test.tsto verify thatemptyTempEncryptionCache()correctly identifies and removes files within the targeted directory.AI Assistance Disclosure
Was AI used to generate this code? Yes, AI was used to assist in identifying the appropriate startup hook locations and to refine the TypeScript implementation for cross-platform compatibility.
Was AI used to generate this PR description? Yes, the pr was generated though was completely read through and made detailed changes to before submitting
Note on GSoC Project Alignment:
The implementation of the
temp_cachedirectory aligns with the architectural goals discussed in all the GSoC proposal's resource handling part of the local encryption implementation agreed by one of the GSOC mentor. This isolated cache provides a structured foundation that can be further used for local encryption tasks and optimized resource handling in future project phases.