Fix/orphaned crypted file cleanup#15098
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughStartup tasks in CLI, desktop and mobile now clear 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: ensure tempDir/encryptionCache exists / list files
FS-->>Resource: file list
loop for each .crypted file
Resource->>FS: unlink <file>
FS-->>Resource: success / error
alt error
Resource->>Logger: warn(error.message)
end
end
Resource-->>App: return (no throw)
App-->>Startup: continue startup tasks
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
|
Hello, this is Claude review for this PR. Please take a look and see what might need to be addressed. A quick read through it and I see indeed there some small issues: Code Review: PR #15098 — Fix orphaned
|
I want to double check on this statement. I intentionally skipped this originally because since for this I would have to add extra peice of code in the Synchronizer while in one of the past pr you have mentioned that you don't want to get the Synchronizer code more messy. Is it fine to add the code there?
I checked the implementation of I will commit the pr including the Synchronizer and all small change but excluding the |
coderabbitai
Bot
added
bug
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/lib/Synchronizer.ts`:
- Around line 777-780: Make cleanup non-fatal and use a deterministic prefix
check: instead of using localResourceContentPath.includes('/encryptionCache/'),
normalize the path and check that it starts with the actual temp
encryption-cache directory (e.g. build a tempDirPrefix from
shim.fsDriver().tempDirPath() + path.sep + 'encryptionCache' and use
path.normalize and string.startsWith(tempDirPrefix)). Wrap the await
shim.fsDriver().remove(localResourceContentPath) call in a try/catch and swallow
errors, emitting a warning (logger.warn or console.warn) rather than letting the
error bubble and fail sync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0912e59d-bb28-4a4e-9e90-19f9e22deb04
📒 Files selected for processing (6)
packages/app-cli/app/app.tspackages/app-desktop/app.tspackages/app-mobile/utils/buildStartupTasks.tspackages/lib/Synchronizer.tspackages/lib/models/Resource.test.tspackages/lib/models/Resource.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/lib/models/Resource.test.ts
- packages/app-mobile/utils/buildStartupTasks.ts
coderabbitai
Bot
removed
bug
| const tempDir = Setting.value('tempDir'); | ||
| const encryptionCacheDir = `${tempDir}/encryptionCache/`; | ||
|
|
||
| if (localResourceContentPath && localResourceContentPath.startsWith(encryptionCacheDir)) { |
There was a problem hiding this comment.
I think this cleanup may need to wrap the temp path rather than only the put. fullPathForSyncUpload() can create the encryptionCache file before we know if the blob needs uploading, but this only removes it after a successful put. So if the upload is skipped, or put fails, the temp file stays until next startup. Maybe a finally around that temp path would be safer?
There was a problem hiding this comment.
Are you suggesting to delete the whole encryption cache here, rather than just the individual item? I don't think that would work, because items can be preuploaded (including doing the encryption) before looping the items to be uploaded
There was a problem hiding this comment.
Ah got you, i meant keeping it per file, only for this same localResourceContentPath, not clearing the whole encryptionCache dir.
So other preuploaded entries would stay as they are, was only thinking about the case where this temp file gets created, then put is skipped or fails.
| if (await this.fsDriver().exists(encryptedPath)) { | ||
| // The file was successfully decrypted into plaintext. | ||
| // We must delete the leftover .crypted file from the main resource directory immediately. | ||
| await this.fsDriver().remove(encryptedPath); |
There was a problem hiding this comment.
Maybe this cleanup should happen after the save below? If encryptedPath is removed here and the save fails/crashes before the DB update, the resource can still look encrypted but no longer have the .crypted file to retry from.
4 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.Previously, these files were not cleaned up automatically, leading to unnecessary disk usage over time.
Solution
This PR introduces a cleanup mechanism by isolating temporary encryption files into a dedicated directory.
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 stored in a dedicated subdirectory:[tempDir]/encryptionCache.Efficient Cleanup: Added startup hooks across Desktop, CLI, and Mobile platforms to clear the encryption cache directory on boot.
Redirection: Updated
Resource.tempCryptedPathso that all temporary encrypted upload artifacts are created inside the encryption cache directory instead of the main resources directory.Upload Cleanup: After a successful upload, temporary encrypted files are deleted if they originate from the encryption cache directory.
Backward Compatibility: Existing handling of
.cryptedfiles in the main resources directory is preserved for pre-upgrade resources.Additional Fix: Ensured proper
awaitusage for file move operations to prevent unhandled promises.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 removes files within the encryption cache directory.AI Assistance Disclosure
Was AI used to generate this code? Yes, AI was used to assist in identifying appropriate startup hook locations and refining the TypeScript implementation for cross-platform compatibility.
Was AI used to generate this PR description? Yes, the PR description was initially AI-assisted and then manually reviewed and refined before submission.
Note on GSoC Project Alignment:
The implementation of the
encryptionCachedirectory aligns with the resource-handling approach discussed in the GSoC local encryption proposal. This isolated directory provides a clean foundation for handling temporary encrypted resources in future encryption-related improvements.