Feature: Files-in-Use #312
Conversation
* first read in-use-file * on failure create/own it
* more static methods * renaming methods
* rename FileIsInUseException to FileAlreadyInUseException * rename InUse:tryMarkInUse to acquire() * add ignore flag for inUse check
* close token on failed file creation * add logging * only move file if file is already created * delete file on close
* add factory method to create InvalidToken * add instance variable owner * define in-use-file-creation threshold as a constant
* create interface * sublass interface by "fake" impl and real impl * decide on cryptofilesystem proeprties what impl to use * create interface UseToken * subclass it with real and stub impl
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java (1)
6-13: Fix Javadoc tags, spelling, and wording consistency.
- Replace invalid with .
- Fix “ciphertextpath” → “ciphertext path”.
- Prefer consistent wording (“used”/“in use”) and punctuation.
Apply:
- * A file is considered <it>in use</it>, if: + * A file is considered <i>in use</i> if: @@ - * @return {@code true} if the file is <it>used</it> by others. {@code false} otherwise + * @return {@code true} if the file is <i>used</i> by others, {@code false} otherwise @@ - * Marks the given ciphertextpath as <em>used</em>. + * Marks the given ciphertext path as <i>used</i>.Also applies to: 20-21, 38-43
🧹 Nitpick comments (5)
src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java (1)
22-24: Avoid no-op defaults on an interface that governs correctness.The default methods silently return “not in use” or a token, masking failures. Prefer a pure interface (no defaults) or throw UnsupportedOperationException by default. This forces explicit semantics in implementations (Real vs Stub).
- default boolean isInUseByOthers(Path ciphertextPath) { - return false; - } + boolean isInUseByOthers(Path ciphertextPath); @@ - default Optional<UseInfo> getUseInfo(Path ciphertextPath) { - return Optional.empty(); - } + Optional<UseInfo> getUseInfo(Path ciphertextPath); @@ - default UseToken use(Path ciphertextPath) throws FileAlreadyInUseException { - return UseToken.INIT_TOKEN; - } + UseToken use(Path ciphertextPath) throws FileAlreadyInUseException; @@ - default void ignoreInUse(Path ciphertextPath) { - - } + void ignoreInUse(Path ciphertextPath);Also applies to: 32-34, 44-46, 48-50
src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java (2)
89-103: Use a shared, daemon scheduler instead of creating a new executor per token.newVirtualThreadPerTaskExecutor() per token is wasteful. Prefer reusing RealInUseManager’s ScheduledExecutorService or a shared daemon executor for delayed creation.
206-210: Remove leftover TODO and rely on the log line.The TODO is obsolete; keep the LOG.info only.
- //ignore - //TODO: LOG - LOG.info("Failed to delete inUse File {}. Must be deleted manually.", path); + LOG.info("Failed to delete inUse File {}. Must be deleted manually.", path);src/main/java/org/cryptomator/cryptofs/inuse/RealInUseManager.java (2)
62-67: Make refresher thread daemon and add a shutdown hook/lifecycle.The dedicated scheduler isn’t daemon and isn’t shut down, risking thread leaks and preventing JVM exit. Use a daemon ThreadFactory and expose close() to shut it down when the filesystem closes.
- this.tokenRefresher = Executors.newSingleThreadScheduledExecutor(); + this.tokenRefresher = Executors.newSingleThreadScheduledExecutor(r -> { + var t = new Thread(r, "inuse-token-refresher"); + t.setDaemon(true); + return t; + });Optionally implement AutoCloseable and call tokenRefresher.shutdownNow() during filesystem teardown.
129-146: validate() now uses owner from file — good. Minor polish.
- Consider content.getProperty(...) for Properties idiom.
- Keep error messages as-is; they’re clear.
- var owner = (String) content.get(UseToken.OWNER_KEY); + var owner = content.getProperty(UseToken.OWNER_KEY); @@ - var stringTime = (String) content.get(UseToken.LASTUPDATED_KEY); + var stringTime = content.getProperty(UseToken.LASTUPDATED_KEY);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java(1 hunks)src/main/java/org/cryptomator/cryptofs/inuse/RealInUseManager.java(1 hunks)src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java(1 hunks)src/test/java/org/cryptomator/cryptofs/inuse/RealInUseManagerTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/org/cryptomator/cryptofs/inuse/RealInUseManagerTest.java
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java (3)
src/main/java/org/cryptomator/cryptofs/common/Constants.java (1)
Constants(11-41)src/main/java/org/cryptomator/cryptofs/common/EncryptedChannels.java (1)
EncryptedChannels(9-22)src/main/java/org/cryptomator/cryptofs/inuse/RealInUseManager.java (1)
RealInUseManager(38-232)
src/main/java/org/cryptomator/cryptofs/inuse/RealInUseManager.java (4)
src/main/java/org/cryptomator/cryptofs/common/CacheUtils.java (1)
CacheUtils(8-57)src/main/java/org/cryptomator/cryptofs/common/Constants.java (1)
Constants(11-41)src/main/java/org/cryptomator/cryptofs/common/EncryptedChannels.java (1)
EncryptedChannels(9-22)src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java (1)
RealUseToken(41-235)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyse
- GitHub Check: Build and Test
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java (1)
111-122: Critical: refresh appends → file grows and may appear stale; must rewrite with TRUNCATE.Refreshing on the same channel without truncate/seek appends new payloads. Over time:
- The .c9u file grows unbounded.
- RealInUseManager.readInUseFile() reads up to INUSE_CLEARTEXT_SIZE (1000), risking truncated parses and outdated lastUpdated → premature “steal”.
Reopen with WRITE+TRUNCATE_EXISTING under the same lock before writing.
Apply:
void refresh() { try { fileCreationSync.lock(); if (!(channel == null || closed)) { - writeInUseFile(); + // Reopen with TRUNCATE to avoid appending and ensure bounded size + try { + channel.close(); + } catch (IOException ignored) { + // ignore + } + var ch = Files.newByteChannel(filePath, Set.of(StandardOpenOption.WRITE, StandardOpenOption.TRUNCATE_EXISTING)); + this.channel = encWrapper.wrapWithEncryption(ch, cryptor); + writeInUseFile(); } } catch (IOException e) { LOG.warn("Failed to update in-use file {}.", filePath, e); } finally { fileCreationSync.unlock(); } }
🧹 Nitpick comments (4)
src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java (4)
185-196: Delete regardless of channel presence; remove stale TODO.Avoid skipping deletion when channel is null and clean up the comment.
- if (channel != null) { - try { - channel.close(); - Files.deleteIfExists(filePath); - } catch (IOException e) { - //ignore - //TODO: LOG - LOG.info("Failed to delete inUse File {}. Must be deleted manually.", path); - } - } + try { + if (channel != null) { + channel.close(); + } + } catch (IOException ignore) { + } + try { + Files.deleteIfExists(filePath); + } catch (IOException e) { + LOG.info("Failed to delete in-use file {}. Must be deleted manually.", path); + } return null;
61-61: Use ReentrantLock instead of a write lock.Read locks aren’t used; a simple ReentrantLock is clearer.
-import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.concurrent.locks.ReentrantLock; @@ - private final ReentrantReadWriteLock.WriteLock fileCreationSync = new ReentrantReadWriteLock().writeLock(); + private final ReentrantLock fileCreationSync = new ReentrantLock();
83-91: Avoid creating a new VirtualThread executor per token.Use the default delayedExecutor or a shared executor; per‑token creation is unnecessary.
- this.creationTask = CompletableFuture.runAsync( - () -> createInUseFile(openOptions), - CompletableFuture.delayedExecutor(Constants.INUSE_DELAY_MILLIS, TimeUnit.MILLISECONDS, Executors.newVirtualThreadPerTaskExecutor())); + this.creationTask = CompletableFuture.runAsync( + () -> createInUseFile(openOptions), + CompletableFuture.delayedExecutor(Constants.INUSE_DELAY_MILLIS, TimeUnit.MILLISECONDS));
127-129: Prefer setProperty over put for Properties.Makes intent explicit and keeps types consistent.
- prop.put(UseToken.OWNER_KEY, owner); - prop.put(UseToken.LASTUPDATED_KEY, Instant.now().toString()); + prop.setProperty(UseToken.OWNER_KEY, owner); + prop.setProperty(UseToken.LASTUPDATED_KEY, Instant.now().toString());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/cryptomator/cryptofs/CryptoFileSystemImpl.java(11 hunks)src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/cryptomator/cryptofs/CryptoFileSystemImpl.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: infeo
PR: cryptomator/cryptofs#312
File: src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java:44-46
Timestamp: 2025-10-14T16:55:45.254Z
Learning: In `src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java`, the interface includes default implementations (including `use()` returning `INIT_TOKEN`) because there are only two implementations—StubInUseManager (stub) and RealInUseManager (real)—with no plans for additional implementations. This design is intentional and acceptable.
📚 Learning: 2025-10-14T16:55:45.254Z
Learnt from: infeo
PR: cryptomator/cryptofs#312
File: src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java:44-46
Timestamp: 2025-10-14T16:55:45.254Z
Learning: In `src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java`, the interface includes default implementations (including `use()` returning `INIT_TOKEN`) because there are only two implementations—StubInUseManager (stub) and RealInUseManager (real)—with no plans for additional implementations. This design is intentional and acceptable.
Applied to files:
src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java
🧬 Code graph analysis (1)
src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java (3)
src/main/java/org/cryptomator/cryptofs/common/Constants.java (1)
Constants(11-41)src/main/java/org/cryptomator/cryptofs/common/EncryptedChannels.java (1)
EncryptedChannels(9-22)src/main/java/org/cryptomator/cryptofs/inuse/RealInUseManager.java (1)
RealInUseManager(38-232)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyse
- GitHub Check: Build and Test
🔇 Additional comments (1)
src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java (1)
33-37: Docs updated to actual on-disk format. LGTM.Now correctly documents Properties with owner/lastUpdated (ISO‑8601).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/java/org/cryptomator/cryptofs/inuse/RealInUseManagerTest.java (1)
358-358: Add direct unit tests for the validate() method.The TODO indicates that the
validate()method inRealInUseManagerlacks dedicated unit tests. While theIsInUseUseInfonested class tests indirectly verify that validUseInfoobjects are produced, there are no tests that directly exercisevalidate()to ensure it properly handles edge cases such as:
- Missing
OWNER_KEYorLASTUPDATED_KEY- Null or blank owner values
- Invalid timestamp formats
- Properties with unexpected keys or malformed content
Do you want me to generate unit tests for the
validate()method to ensure comprehensive coverage of these edge cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/org/cryptomator/cryptofs/DirectoryIdBackup.java(3 hunks)src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java(1 hunks)src/main/java/org/cryptomator/cryptofs/inuse/RealInUseManager.java(1 hunks)src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java(1 hunks)src/main/java/org/cryptomator/cryptofs/inuse/package-info.java(1 hunks)src/test/java/org/cryptomator/cryptofs/inuse/RealInUseManagerTest.java(1 hunks)src/test/java/org/cryptomator/cryptofs/inuse/RealUseTokenTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/cryptomator/cryptofs/inuse/package-info.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java
- src/test/java/org/cryptomator/cryptofs/inuse/RealUseTokenTest.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: infeo
PR: cryptomator/cryptofs#312
File: src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java:44-46
Timestamp: 2025-10-14T16:55:45.287Z
Learning: In `src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java`, the interface includes default implementations (including `use()` returning `INIT_TOKEN`) because there are only two implementations—StubInUseManager (stub) and RealInUseManager (real)—with no plans for additional implementations. This design is intentional and acceptable.
📚 Learning: 2025-10-14T16:55:45.287Z
Learnt from: infeo
PR: cryptomator/cryptofs#312
File: src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java:44-46
Timestamp: 2025-10-14T16:55:45.287Z
Learning: In `src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java`, the interface includes default implementations (including `use()` returning `INIT_TOKEN`) because there are only two implementations—StubInUseManager (stub) and RealInUseManager (real)—with no plans for additional implementations. This design is intentional and acceptable.
Applied to files:
src/test/java/org/cryptomator/cryptofs/inuse/RealInUseManagerTest.javasrc/main/java/org/cryptomator/cryptofs/inuse/RealInUseManager.javasrc/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java
🧬 Code graph analysis (4)
src/main/java/org/cryptomator/cryptofs/DirectoryIdBackup.java (1)
src/main/java/org/cryptomator/cryptofs/common/EncryptedChannels.java (1)
EncryptedChannels(9-22)
src/test/java/org/cryptomator/cryptofs/inuse/RealInUseManagerTest.java (3)
src/main/java/org/cryptomator/cryptofs/common/EncryptedChannels.java (1)
EncryptedChannels(9-22)src/main/java/org/cryptomator/cryptofs/inuse/RealInUseManager.java (1)
RealInUseManager(38-229)src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java (1)
RealUseToken(35-206)
src/main/java/org/cryptomator/cryptofs/inuse/RealInUseManager.java (4)
src/main/java/org/cryptomator/cryptofs/common/CacheUtils.java (1)
CacheUtils(8-57)src/main/java/org/cryptomator/cryptofs/common/Constants.java (1)
Constants(11-41)src/main/java/org/cryptomator/cryptofs/common/EncryptedChannels.java (1)
EncryptedChannels(9-22)src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java (1)
RealUseToken(35-206)
src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java (3)
src/main/java/org/cryptomator/cryptofs/common/Constants.java (1)
Constants(11-41)src/main/java/org/cryptomator/cryptofs/common/EncryptedChannels.java (1)
EncryptedChannels(9-22)src/main/java/org/cryptomator/cryptofs/inuse/RealInUseManager.java (1)
RealInUseManager(38-229)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyse
- GitHub Check: Build and Test
🔇 Additional comments (10)
src/main/java/org/cryptomator/cryptofs/DirectoryIdBackup.java (3)
5-5: LGTM! Clean refactoring to centralized utility.The import addition supports the refactoring that eliminates local wrapper methods in favor of the centralized
EncryptedChannelsutility class. This reduces code duplication and improves maintainability.
37-42: LGTM! Correct delegation to centralized utility.The
writemethod now correctly delegates encryption channel creation toEncryptedChannels.wrapEncryptionAround, eliminating the local wrapper method. The method signature and behavior are preserved.
65-83: LGTM! Correct delegation to centralized utility.The
readmethod now correctly delegates decryption channel creation toEncryptedChannels.wrapDecryptionAround, eliminating the local wrapper method. The method signature and behavior are preserved, and the refactoring aligns with the broader PR objective to centralize channel provisioning.src/main/java/org/cryptomator/cryptofs/inuse/RealInUseManager.java (4)
129-129: Address the TODO for validate() testing.This TODO corresponds to line 358 in
RealInUseManagerTest.javawhere direct unit tests for thevalidate()method are missing. Consider adding comprehensive unit tests to ensure all edge cases (missing keys, null/blank values, invalid timestamps) are properly covered.Based on the TODO comment in the test file.
50-67: LGTM!The constructor properly initializes caches with reasonable TTLs (2 minutes for ignored files, 5 seconds for use info) and sets up scheduled refresh of tokens every 5 minutes. The cache sizes (100 for ignored, 1000 for useInfo) are appropriate for the use case.
110-127: LGTM!The
readInUseFile()method correctly:
- Allocates a fixed-size buffer (
INUSE_CLEARTEXT_SIZE)- Opens and wraps the channel with decryption
- Checks for empty files (readBytes < 0)
- Loads Properties from the decrypted content
186-202: LGTM!The
createInternal()method properly handles all file state scenarios:
- Existing valid in-use file → throws
FileAlreadyInUseException- Non-existent file → creates new token with
CREATE_NEW- Invalid file → steals ownership with
TRUNCATE_EXISTING- IO errors → wraps as
UncheckedIOExceptionCache invalidation on successful creation is correct.
src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java (3)
62-86: LGTM!The constructor correctly:
- Maps
ActivationTypeto appropriateOpenOptionsets (CREATE→CREATE_NEW,STEAL→TRUNCATE_EXISTING)- Handles
NONEactivation by marking as closed immediately- Schedules delayed creation after
INUSE_DELAY_MILLISusing a virtual thread executor
135-161: LGTM!The
moveToInternal()method correctly:
- Synchronizes with the
fileCreationSynclock- Only moves the file if the channel exists (file was persisted)
- Updates both the map and the
filePathfield atomically- Handles errors by logging and closing to prevent invalid states
168-194: LGTM!The
close()method properly:
- Prevents double-close with the
closedflag- Cancels the delayed creation task
- Closes the channel and deletes the file atomically within the map compute
- Logs failures without throwing (appropriate for cleanup code)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java (2)
168-171: Include exception in warn log for deletion failureCurrent log drops the exception; add it for diagnostics.
- LOG.warn("Failed to delete inUse File {}. Must be deleted manually.", path); + LOG.warn("Failed to delete in-use file {}. Must be deleted manually.", path, e);
52-53: Use ReentrantLock instead of WriteLock (simpler and clearer)A plain mutual-exclusion is sufficient; ReentrantLock reduces complexity and intent is clearer.
If acceptable, I can provide a small patch to switch to ReentrantLock.
src/test/java/org/cryptomator/cryptofs/inuse/RealUseTokenTest.java (1)
30-41: Add tests for refresh() null-safety (before creation and after close)Current tests don’t cover calling refresh when channel is null. This would have caught the NPE in RealUseToken.refresh().
Add:
@Test @DisplayName("refresh before creation does not throw") public void testRefreshBeforeCreation() { var filePath = tmpDir.resolve("inUse.file"); try (var token = new RealUseToken(filePath, "test3000", cryptor, useTokens, StandardOpenOption.CREATE_NEW, encWrapper)) { // Immediately refresh before delayed creation -> no NPE Assertions.assertDoesNotThrow(token::refresh); } } @Test @DisplayName("refresh after close does not throw") public void testRefreshAfterClose() { var filePath = tmpDir.resolve("inUse.file"); var token = new RealUseToken(filePath, "test3000", cryptor, useTokens, StandardOpenOption.CREATE_NEW, encWrapper); token.close(); Assertions.assertDoesNotThrow(token::refresh); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java(1 hunks)src/test/java/org/cryptomator/cryptofs/inuse/RealUseTokenTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: infeo
PR: cryptomator/cryptofs#312
File: src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java:44-46
Timestamp: 2025-10-14T16:55:45.287Z
Learning: In `src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java`, the interface includes default implementations (including `use()` returning `INIT_TOKEN`) because there are only two implementations—StubInUseManager (stub) and RealInUseManager (real)—with no plans for additional implementations. This design is intentional and acceptable.
📚 Learning: 2025-10-14T16:55:45.287Z
Learnt from: infeo
PR: cryptomator/cryptofs#312
File: src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java:44-46
Timestamp: 2025-10-14T16:55:45.287Z
Learning: In `src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java`, the interface includes default implementations (including `use()` returning `INIT_TOKEN`) because there are only two implementations—StubInUseManager (stub) and RealInUseManager (real)—with no plans for additional implementations. This design is intentional and acceptable.
Applied to files:
src/test/java/org/cryptomator/cryptofs/inuse/RealUseTokenTest.javasrc/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java
🧬 Code graph analysis (2)
src/test/java/org/cryptomator/cryptofs/inuse/RealUseTokenTest.java (1)
src/main/java/org/cryptomator/cryptofs/common/Constants.java (1)
Constants(11-41)
src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java (3)
src/main/java/org/cryptomator/cryptofs/common/Constants.java (1)
Constants(11-41)src/main/java/org/cryptomator/cryptofs/common/EncryptedChannels.java (1)
EncryptedChannels(9-22)src/main/java/org/cryptomator/cryptofs/inuse/RealInUseManager.java (1)
RealInUseManager(38-229)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyse
- GitHub Check: Build and Test
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java (1)
6-13: Clarify "in use" definition in interface Javadoc.The bullets describe what InUseManager does but don't define when a file is considered "in use by others." Per the implementation, a file is in use when: (1) an in-use file exists, (2) it's owned by a different owner than this filesystem, and (3) lastUpdated is within the staleness threshold. Rephrase the first bullet to make this explicit.
Based on past review comments.
src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java (2)
108-118: Add defensive size check for Properties payload.If the serialized Properties exceeds
INUSE_CLEARTEXT_SIZE(e.g., long owner string), readers will silently truncate, causing validation failures. Add a check after line 115 to log a warning or throw ifrawInfo.size() > Constants.INUSE_CLEARTEXT_SIZE.Apply this diff:
int writeInUseFile() throws IOException { try (var nonClosingWrapper = new NonClosingByteChannel(channel); // var encChannel = encWrapper.wrapWithEncryption(nonClosingWrapper, cryptor)) { var rawInfo = new ByteArrayOutputStream(Constants.INUSE_CLEARTEXT_SIZE); var prop = new Properties(); prop.put(UseToken.OWNER_KEY, owner); prop.put(UseToken.LASTUPDATED_KEY, Instant.now().toString()); prop.store(rawInfo, null); + var data = rawInfo.toByteArray(); + if (data.length > Constants.INUSE_CLEARTEXT_SIZE) { + throw new IOException("In-use file payload (%d bytes) exceeds buffer size (%d bytes). Shorten owner string.".formatted(data.length, Constants.INUSE_CLEARTEXT_SIZE)); + } - return encChannel.write(ByteBuffer.wrap(rawInfo.toByteArray())); + return encChannel.write(ByteBuffer.wrap(data)); } }Based on past review comments.
93-106: Critical: refresh() positions channel AFTER write, leaving trailing bytes.Line 100 calls
channel.position(0)AFTERwriteInUseFile()completes, which doesn't prevent stale data. If the new Properties payload is shorter than the previous write, trailing bytes will remain and corrupt the file. Position and truncate the channel BEFORE writing.Apply this diff:
void refresh() { fileCreationSync.lock(); try { if (closed || channel == null) { return; } + channel.position(0); + channel.truncate(0); writeInUseFile(); - channel.position(0); } catch (IOException e) { LOG.debug("Failed to refresh in-use file {}.", filePath, e); } finally { fileCreationSync.unlock(); } }
🧹 Nitpick comments (1)
src/main/java/org/cryptomator/cryptofs/inuse/RealInUseManager.java (1)
132-132: Remove stale TODO comment.The
validate()method is already tested viaRealInUseManagerTest(nested classIsInUseUseInfoand other test methods that callreadInUseFile()orisInUse()which invokevalidate()). Remove the TODO.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java(9 hunks)src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java(1 hunks)src/main/java/org/cryptomator/cryptofs/inuse/RealInUseManager.java(1 hunks)src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java(1 hunks)src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java(7 hunks)src/test/java/org/cryptomator/cryptofs/inuse/RealInUseManagerTest.java(1 hunks)src/test/java/org/cryptomator/cryptofs/inuse/RealUseTokenTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: infeo
PR: cryptomator/cryptofs#312
File: src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java:44-46
Timestamp: 2025-10-14T16:55:45.287Z
Learning: In `src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java`, the interface includes default implementations (including `use()` returning `INIT_TOKEN`) because there are only two implementations—StubInUseManager (stub) and RealInUseManager (real)—with no plans for additional implementations. This design is intentional and acceptable.
📚 Learning: 2025-10-14T16:55:45.287Z
Learnt from: infeo
PR: cryptomator/cryptofs#312
File: src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java:44-46
Timestamp: 2025-10-14T16:55:45.287Z
Learning: In `src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java`, the interface includes default implementations (including `use()` returning `INIT_TOKEN`) because there are only two implementations—StubInUseManager (stub) and RealInUseManager (real)—with no plans for additional implementations. This design is intentional and acceptable.
Applied to files:
src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.javasrc/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.javasrc/main/java/org/cryptomator/cryptofs/inuse/RealInUseManager.javasrc/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.javasrc/test/java/org/cryptomator/cryptofs/inuse/RealInUseManagerTest.java
🧬 Code graph analysis (4)
src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java (3)
src/main/java/org/cryptomator/cryptofs/common/Constants.java (1)
Constants(11-41)src/main/java/org/cryptomator/cryptofs/common/EncryptedChannels.java (1)
EncryptedChannels(9-22)src/main/java/org/cryptomator/cryptofs/inuse/RealInUseManager.java (1)
RealInUseManager(39-233)
src/main/java/org/cryptomator/cryptofs/inuse/RealInUseManager.java (4)
src/main/java/org/cryptomator/cryptofs/common/CacheUtils.java (1)
CacheUtils(8-57)src/main/java/org/cryptomator/cryptofs/common/Constants.java (1)
Constants(11-41)src/main/java/org/cryptomator/cryptofs/common/EncryptedChannels.java (1)
EncryptedChannels(9-22)src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java (1)
RealUseToken(36-211)
src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java (2)
src/main/java/org/cryptomator/cryptofs/inuse/FileAlreadyInUseException.java (1)
FileAlreadyInUseException(6-11)src/main/java/org/cryptomator/cryptofs/EffectiveOpenOptions.java (1)
EffectiveOpenOptions(29-189)
src/test/java/org/cryptomator/cryptofs/inuse/RealInUseManagerTest.java (3)
src/main/java/org/cryptomator/cryptofs/common/EncryptedChannels.java (1)
EncryptedChannels(9-22)src/main/java/org/cryptomator/cryptofs/inuse/RealInUseManager.java (1)
RealInUseManager(39-233)src/main/java/org/cryptomator/cryptofs/inuse/RealUseToken.java (1)
RealUseToken(36-211)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyse
- GitHub Check: Build and Test
🔇 Additional comments (3)
src/test/java/org/cryptomator/cryptofs/inuse/RealUseTokenTest.java (1)
1-257: LGTM! Comprehensive test coverage for RealUseToken.The test suite thoroughly exercises token lifecycle, including delayed persistence, file operations, move semantics, refresh behavior, and error handling. The use of Awaitility for timing assertions and extensive mocking is appropriate for this component.
src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java (1)
68-271: LGTM! Proper integration testing for in-use feature.The updated tests correctly verify InUseManager integration with OpenCryptoFile, covering token lifecycle scenarios, read-only bypass, token acquisition on closed state, exception handling, and path update operations.
src/test/java/org/cryptomator/cryptofs/inuse/RealInUseManagerTest.java (1)
1-367: LGTM! Thorough test coverage for RealInUseManager.The test suite comprehensively exercises all RealInUseManager paths, including cache interactions, validation logic, token creation strategies, error handling, and edge cases. The nested test structure and extensive mocking appropriately test the component in isolation.
| this.tokenPersistor = Executors.newVirtualThreadPerTaskExecutor(); | ||
| this.tokenRefresher = Executors.newSingleThreadScheduledExecutor(); //TODO: never closed -> resource leak |
There was a problem hiding this comment.
Resource leak: ExecutorServices never shut down.
Both tokenPersistor and tokenRefresher are never closed, leaking threads and preventing clean shutdown. Implement AutoCloseable or add an explicit cleanup method to shut down both executors when the filesystem is closed.
Consider:
public void close() {
tokenRefresher.shutdown();
tokenPersistor.shutdown();
try {
if (!tokenRefresher.awaitTermination(5, TimeUnit.SECONDS)) {
tokenRefresher.shutdownNow();
}
if (!tokenPersistor.awaitTermination(5, TimeUnit.SECONDS)) {
tokenPersistor.shutdownNow();
}
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
tokenRefresher.shutdownNow();
tokenPersistor.shutdownNow();
}
}Then wire this into the CryptoFileSystem lifecycle.
🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/cryptofs/inuse/RealInUseManager.java around
lines 64-65, the Executors (tokenPersistor and tokenRefresher) are never shut
down causing a thread/resource leak; implement a cleanup by making the class
AutoCloseable or adding a close() method that calls shutdown() on both
executors, awaits termination with a timeout (e.g. 5s), calls shutdownNow() if
they don't terminate, and handles InterruptedException by re-setting the
interrupt flag and forcing shutdownNow(); finally, wire this close() into the
CryptoFileSystem shutdown lifecycle so the executors are always stopped when the
filesystem is closed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pom.xml (1)
162-167: Dependency addition is well‑justified for async testing.Awaitility is an appropriate choice for testing the asynchronous state transitions in the in‑use feature (delayed persistence, refresh intervals, token lifecycle). The dependency is properly scoped and pinned.
For consistency with other test dependencies (junit, mockito, hamcrest, jimfs), consider defining the version as a property variable:
<awaitility.version>4.3.0</awaitility.version>Then reference it in the dependency. This is a minor style preference and can be addressed now or deferred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pom.xml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: infeo
PR: cryptomator/cryptofs#312
File: src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java:44-46
Timestamp: 2025-10-14T16:55:45.287Z
Learning: In `src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java`, the interface includes default implementations (including `use()` returning `INIT_TOKEN`) because there are only two implementations—StubInUseManager (stub) and RealInUseManager (real)—with no plans for additional implementations. This design is intentional and acceptable.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyse
- GitHub Check: Build and Test
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.java (2)
654-668: Remove unused mock variable.The
inUsePathmock declared on line 656 is never wired into the test setup (nowhen()stubs) and the verification on line 667 doesn't add meaningful coverage since it's not connected to any code path.Apply this diff:
@Test public void testDeleteShortenedExistingFile() throws IOException { - var inUsePath = mock(Path.class, "in use file"); when(cryptoPathMapper.getCiphertextFileType(cleartextPath)).thenReturn(CiphertextFileType.FILE); when(physicalFsProv.deleteIfExists(ciphertextRawPath)).thenReturn(true); doNothing().when(openCryptoFiles).delete(Mockito.any()); when(ciphertextPath.isShortened()).thenReturn(true); inTest.delete(cleartextPath); verify(readonlyFlag).assertWritable(); verify(openCryptoFiles).delete(ciphertextFilePath); verify(physicalFsProv).deleteIfExists(ciphertextRawPath); - verify(physicalFsProv, never()).deleteIfExists(inUsePath); }
670-685: Remove unused mock variable.The
inUsePathmock declared on line 672 is never wired into the test setup and the verification on line 684 doesn't add meaningful coverage.Apply this diff:
@Test public void testDeleteInUseFileThrows() throws IOException { - var inUsePath = mock(Path.class, "in use file"); when(cryptoPathMapper.getCiphertextFileType(cleartextPath)).thenReturn(CiphertextFileType.FILE); when(physicalFsProv.deleteIfExists(ciphertextRawPath)).thenReturn(true); doNothing().when(openCryptoFiles).delete(Mockito.any()); when(ciphertextPath.isShortened()).thenReturn(false); var inTestSpy = spy(inTest); doThrow(FileAlreadyInUseException.class).when(inTestSpy).checkUsage(cleartextPath, ciphertextPath); Assertions.assertThrows(FileAlreadyInUseException.class, () -> inTestSpy.delete(cleartextPath)); verify(openCryptoFiles, never()).delete(ciphertextFilePath); verify(physicalFsProv, never()).deleteIfExists(ciphertextRawPath); - verify(physicalFsProv, never()).deleteIfExists(inUsePath); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/cryptomator/cryptofs/event/FileIsInUseEvent.java(1 hunks)src/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.java(9 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: infeo
PR: cryptomator/cryptofs#312
File: src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java:44-46
Timestamp: 2025-10-14T16:55:45.287Z
Learning: In `src/main/java/org/cryptomator/cryptofs/inuse/InUseManager.java`, the interface includes default implementations (including `use()` returning `INIT_TOKEN`) because there are only two implementations—StubInUseManager (stub) and RealInUseManager (real)—with no plans for additional implementations. This design is intentional and acceptable.
📚 Learning: 2025-10-17T13:54:22.498Z
Learnt from: infeo
PR: cryptomator/cryptofs#319
File: src/main/java/org/cryptomator/cryptofs/event/BrokenFileNodeEvent.java:14-18
Timestamp: 2025-10-17T13:54:22.498Z
Learning: In the cryptofs event package (e.g., BrokenFileNodeEvent, ConflictResolutionFailedEvent, ConflictResolvedEvent), the event constructors are intended to be called only from within the filesystem implementation, so null-safety guards like Objects.requireNonNull() are not needed because the call sites are controlled and paths are guaranteed to be non-null.
Applied to files:
src/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.javasrc/main/java/org/cryptomator/cryptofs/event/FileIsInUseEvent.java
🧬 Code graph analysis (1)
src/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.java (2)
src/main/java/org/cryptomator/cryptofs/inuse/FileAlreadyInUseException.java (1)
FileAlreadyInUseException(6-11)src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFiles.java (1)
TwoPhaseMove(124-174)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyse
- GitHub Check: Build and Test
🔇 Additional comments (3)
src/main/java/org/cryptomator/cryptofs/event/FileIsInUseEvent.java (1)
1-25: LGTM!The event record is well-structured. The secondary constructor conveniently accepts a
PathforcleartextPathand converts it toStringfor the canonical constructor, which is a good ergonomic choice for call sites.src/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.java (2)
589-597: LGTM!The test correctly verifies that when a file channel cannot be opened due to in-use status, both the exception is propagated and the corresponding event is emitted.
850-902: LGTM!The move tests properly verify in-use checking for both source and destination paths. The refactoring to use a helper method
moveFileWithXInUseeliminates duplication between the two in-use test cases.
| @Test | ||
| @DisplayName("checkUsage throws exception when file is in-use") | ||
| public void testCheckUsageForNotInUseFiles() throws FileAlreadyInUseException { | ||
| CryptoPath cleartextPath = mock(CryptoPath.class, "cleartext"); | ||
| CryptoPath ciphertextFilePath = mock(CryptoPath.class, "d/00/00/path.c9r"); | ||
| CiphertextFilePath ciphertextPath = mock(CiphertextFilePath.class); | ||
| when(ciphertextPath.getFilePath()).thenReturn(ciphertextFilePath); | ||
|
|
||
| when(inUseManager.isInUseByOthers(ciphertextFilePath)).thenReturn(false); | ||
|
|
||
| Assertions.assertDoesNotThrow(() -> inTest.checkUsage(cleartextPath, ciphertextPath)); | ||
| verify(inUseManager).isInUseByOthers(ciphertextFilePath); | ||
| verify(eventConsumer, never()).accept(any()); | ||
| } |
There was a problem hiding this comment.
Fix the incorrect @DisplayName annotation.
The display name says "throws exception when file is in-use" but the test name (testCheckUsageForNotInUseFiles) and body (using assertDoesNotThrow) clearly test the opposite case where the file is NOT in use.
Apply this diff:
@Test
- @DisplayName("checkUsage throws exception when file is in-use")
+ @DisplayName("checkUsage succeeds when file is not in-use")
public void testCheckUsageForNotInUseFiles() throws FileAlreadyInUseException {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| @DisplayName("checkUsage throws exception when file is in-use") | |
| public void testCheckUsageForNotInUseFiles() throws FileAlreadyInUseException { | |
| CryptoPath cleartextPath = mock(CryptoPath.class, "cleartext"); | |
| CryptoPath ciphertextFilePath = mock(CryptoPath.class, "d/00/00/path.c9r"); | |
| CiphertextFilePath ciphertextPath = mock(CiphertextFilePath.class); | |
| when(ciphertextPath.getFilePath()).thenReturn(ciphertextFilePath); | |
| when(inUseManager.isInUseByOthers(ciphertextFilePath)).thenReturn(false); | |
| Assertions.assertDoesNotThrow(() -> inTest.checkUsage(cleartextPath, ciphertextPath)); | |
| verify(inUseManager).isInUseByOthers(ciphertextFilePath); | |
| verify(eventConsumer, never()).accept(any()); | |
| } | |
| @Test | |
| @DisplayName("checkUsage succeeds when file is not in-use") | |
| public void testCheckUsageForNotInUseFiles() throws FileAlreadyInUseException { | |
| CryptoPath cleartextPath = mock(CryptoPath.class, "cleartext"); | |
| CryptoPath ciphertextFilePath = mock(CryptoPath.class, "d/00/00/path.c9r"); | |
| CiphertextFilePath ciphertextPath = mock(CiphertextFilePath.class); | |
| when(ciphertextPath.getFilePath()).thenReturn(ciphertextFilePath); | |
| when(inUseManager.isInUseByOthers(ciphertextFilePath)).thenReturn(false); | |
| Assertions.assertDoesNotThrow(() -> inTest.checkUsage(cleartextPath, ciphertextPath)); | |
| verify(inUseManager).isInUseByOthers(ciphertextFilePath); | |
| verify(eventConsumer, never()).accept(any()); | |
| } |
🤖 Prompt for AI Agents
In src/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.java around
lines 460 to 473, the @DisplayName annotation is incorrect (it says "throws
exception when file is in-use") while the test method name and assertions verify
the NOT-in-use case; update the @DisplayName value to accurately describe the
test (for example: "does not throw when file is not in use" or "checkUsage does
not throw if file is not in use") so it matches the test name and body.
1 participant
This PR implements the Files-in-use feature.
Problem
Cryptofs encrypts each file by its own, in order to be easily used for often synced files (i.e. synced with a cloud storage provider). A common scenario nowadays is, that multiple people work on the same synced files at the same time but at different locations. This leads to conflicts, which are normally resolved by the sync algorithms (and Cryptomator. But some applications simply overwrite newer versions without asking. Also, if the files reside on a network shared location, you have direct access and hence conflicts will not be detected.
Solution
Every time a file channel is opened or an existing file is overwritten/deleted, we first check if the file is currently used by another party. "Used" means, that an encrypted helper file with the same cipher file name and file extension
.c9uexists, containing anownerstring and alastUpdatedtimestamp. Ifthe file is considered as in use. If the file is used, io operations throw a special exception (
FileAlreadyInUseException). Additionally, an event is triggered.An in-use file is created, if a file channel with WRITE option is successfully opened to a file. The owner is set to the string handed over in the
CryptoFileSystemProperties. The inUseFile has the same lifetime as the OpenCryptoFile and if the latter is closed, also the inUseFile is closed (and deleted). To mitigate performance drops, the first 5 seconds the inUseFile only exists in memory and afterwards it is persisted on disk. If during the "virtual" time the file channel is closed again , no file is created.Every 5min, all open inUseFiles are refreshed, i.e. rewritten with an updated "lastUpdated" timestamp. If the lastUpdated time is 10min ago, it is considered stale and the file can be stolen by other users.
If a file is in-use , the consumer can "unblock" this state for the next accesses over a short amount of time. If during this time a file channel is openened, any existing inUseFile will be stolen, i.e. after successful opening the file is overwritten with this cryptofilesystem owner.
The whole inUse system is deactivated if no filesystem owner is specified in the CryptoFileSystemProperties. This is done by using a
StubInUseManager.Todos