From 3ce8d496e09e31c82e71632cf243a0cfc07a2357 Mon Sep 17 00:00:00 2001 From: CounterClops <19741838+CounterClops@users.noreply.github.com> Date: Thu, 11 Dec 2025 06:52:37 +1100 Subject: [PATCH] Fix issues with library scans and file moves with a focus on SMB usage (#1744) * fix: add retry and retry delays to handle smb lag * fix: issue with importing cb7 files. missing required dependency. Scan would fail prematurely if a cb7 file was present * fix: cleanup empty subdirectories when cleaning up parent folders * fix: update folder cleanup to account for SMB delays * chore: cleanup redundant and weak tests from auto-generated unit tests * fix: make sleep function name more descriptive * fix: minor optimisation around path matching * fix: update xz package to latest version --- booklore-api/build.gradle | 1 + .../booklore/service/file/FileMoveHelper.java | 166 +++++++- .../service/file/FileMoveService.java | 161 ++++---- .../service/library/FileAsBookProcessor.java | 14 +- .../MonitoringRegistrationService.java | 20 + .../service/monitoring/MonitoringService.java | 52 ++- .../service/file/FileMoveHelperTest.java | 365 ++++++++++++++++++ 7 files changed, 682 insertions(+), 97 deletions(-) create mode 100644 booklore-api/src/test/java/com/adityachandel/booklore/service/file/FileMoveHelperTest.java diff --git a/booklore-api/build.gradle b/booklore-api/build.gradle index ade8a418f..49400fee8 100644 --- a/booklore-api/build.gradle +++ b/booklore-api/build.gradle @@ -72,6 +72,7 @@ dependencies { // --- API Documentation --- implementation 'org.springdoc:springdoc-openapi-starter-webmvc-ui:2.8.14' implementation 'org.apache.commons:commons-compress:1.28.0' + implementation 'org.tukaani:xz:1.11' // Required by commons-compress for 7z support implementation 'org.apache.commons:commons-text:1.14.0' // --- Template Engine --- diff --git a/booklore-api/src/main/java/com/adityachandel/booklore/service/file/FileMoveHelper.java b/booklore-api/src/main/java/com/adityachandel/booklore/service/file/FileMoveHelper.java index 86846bf03..bb3cc2d9a 100644 --- a/booklore-api/src/main/java/com/adityachandel/booklore/service/file/FileMoveHelper.java +++ b/booklore-api/src/main/java/com/adityachandel/booklore/service/file/FileMoveHelper.java @@ -12,7 +12,11 @@ import org.springframework.stereotype.Component; import java.io.File; import java.io.IOException; +import java.nio.file.AccessDeniedException; +import java.nio.file.DirectoryNotEmptyException; +import java.nio.file.FileSystemException; import java.nio.file.Files; +import java.nio.file.NoSuchFileException; import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.StandardCopyOption; @@ -27,37 +31,85 @@ public class FileMoveHelper { private final MonitoringRegistrationService monitoringRegistrationService; private final AppSettingService appSettingService; + private static final int MAX_ATTEMPTS = 3; + private static final long RETRY_DELAY_MS = 100; + private static final Set> RETRYABLE_EXCEPTIONS = Set.of( + NoSuchFileException.class, + AccessDeniedException.class, + FileSystemException.class, + DirectoryNotEmptyException.class + ); + private static final Set IGNORED_FILENAMES = Set.of(".DS_Store", "Thumbs.db"); + + public boolean waitForFileAccessible(Path path) { + for (int attempt = 1; attempt <= MAX_ATTEMPTS; attempt++) { + if (Files.exists(path) && Files.isReadable(path)) { + return true; + } + if (attempt < MAX_ATTEMPTS) { + log.debug("File not accessible on attempt {}/{}, waiting {}ms: {}", + attempt, MAX_ATTEMPTS, RETRY_DELAY_MS, path); + waitBeforeRetry(); + } + } + return false; + } + public void moveFile(Path source, Path target) throws IOException { + if (!waitForFileAccessible(source)) { + throw new NoSuchFileException(source.toString(), null, "Source file not accessible after retries"); + } + if (target.getParent() != null) { Files.createDirectories(target.getParent()); } log.info("Moving file from {} to {}", source, target); - Files.move(source, target, StandardCopyOption.REPLACE_EXISTING); + executeWithRetry(() -> Files.move(source, target, StandardCopyOption.REPLACE_EXISTING)); } - public Path moveFileWithBackup(Path source, Path target) throws IOException { + public Path moveFileWithBackup(Path source) throws IOException { + if (!waitForFileAccessible(source)) { + throw new NoSuchFileException(source.toString(), null, "Source file not accessible after retries"); + } + Path tempPath = source.resolveSibling(source.getFileName().toString() + ".tmp_move"); log.info("Moving file from {} to temporary location {}", source, tempPath); - Files.move(source, tempPath, StandardCopyOption.REPLACE_EXISTING); + executeWithRetry(() -> Files.move(source, tempPath, StandardCopyOption.REPLACE_EXISTING)); return tempPath; } public void commitMove(Path tempPath, Path target) throws IOException { + if (!waitForFileAccessible(tempPath)) { + throw new NoSuchFileException(tempPath.toString(), null, "Temporary file not accessible before commit"); + } + if (target.getParent() != null) { Files.createDirectories(target.getParent()); } log.info("Committing move from temporary location {} to {}", tempPath, target); - Files.move(tempPath, target, StandardCopyOption.REPLACE_EXISTING); + executeWithRetry(() -> Files.move(tempPath, target, StandardCopyOption.REPLACE_EXISTING)); } public void rollbackMove(Path tempPath, Path originalSource) { - if (Files.exists(tempPath)) { + if (!Files.exists(tempPath)) { + return; + } + + for (int attempt = 1; attempt <= MAX_ATTEMPTS; attempt++) { try { log.info("Rolling back move from {} to {}", tempPath, originalSource); Files.move(tempPath, originalSource, StandardCopyOption.REPLACE_EXISTING); + return; } catch (IOException e) { - log.error("Failed to rollback file move from {} to {}", tempPath, originalSource, e); + if (attempt == MAX_ATTEMPTS) { + log.error("Failed to rollback file move from {} to {} after {} attempts. " + + "Orphaned temp file may need manual cleanup: {}", + tempPath, originalSource, MAX_ATTEMPTS, tempPath, e); + return; + } + log.warn("Rollback attempt {}/{} failed, retrying: {}", attempt, MAX_ATTEMPTS, e.getMessage()); + waitBeforeRetry(); } } } @@ -108,9 +160,7 @@ public class FileMoveHelper { } public void deleteEmptyParentDirsUpToLibraryFolders(Path currentDir, Set libraryRoots) { - Path dir = currentDir; - Set ignoredFilenames = Set.of(".DS_Store", "Thumbs.db"); - dir = dir.toAbsolutePath().normalize(); + Path dir = currentDir.toAbsolutePath().normalize(); Set normalizedRoots = new HashSet<>(); for (Path root : libraryRoots) { normalizedRoots.add(root.toAbsolutePath().normalize()); @@ -119,13 +169,7 @@ public class FileMoveHelper { if (isLibraryRoot(dir, normalizedRoots)) { break; } - File[] files = dir.toFile().listFiles(); - if (files == null) { - log.warn("Cannot read directory: {}. Stopping cleanup.", dir); - break; - } - if (hasOnlyIgnoredFiles(files, ignoredFilenames)) { - deleteIgnoredFilesAndDirectory(files, dir); + if (deleteIfEffectivelyEmpty(dir, normalizedRoots)) { dir = dir.getParent(); } else { break; @@ -133,6 +177,53 @@ public class FileMoveHelper { } } + private boolean deleteIfEffectivelyEmpty(Path dir, Set libraryRoots) { + if (isLibraryRoot(dir, libraryRoots)) { + return false; + } + + File[] contents = dir.toFile().listFiles(); + if (contents == null) { + log.warn("Cannot read directory: {}. Stopping cleanup.", dir); + return false; + } + + boolean deletedAnySubdirectory = recursivelyDeleteEmptySubdirectories(contents, libraryRoots); + + if (deletedAnySubdirectory) { + waitBeforeRetry(); + } + + File[] remainingContents = dir.toFile().listFiles(); + if (remainingContents == null) { + log.warn("Cannot read directory after subdirectory cleanup: {}", dir); + return false; + } + + if (isSafeToDelete(remainingContents)) { + deleteIgnoredFilesAndDirectory(remainingContents, dir); + return true; + } + + return false; + } + + private boolean recursivelyDeleteEmptySubdirectories(File[] contents, Set libraryRoots) { + boolean deletedAny = false; + for (File file : contents) { + if (isNonSymlinkDirectory(file)) { + if (deleteIfEffectivelyEmpty(file.toPath(), libraryRoots)) { + deletedAny = true; + } + } + } + return deletedAny; + } + + private boolean isNonSymlinkDirectory(File file) { + return file.isDirectory() && !Files.isSymbolicLink(file.toPath()); + } + private boolean isLibraryRoot(Path currentDir, Set normalizedRoots) { for (Path root : normalizedRoots) { try { @@ -146,9 +237,11 @@ public class FileMoveHelper { return false; } - private boolean hasOnlyIgnoredFiles(File[] files, Set ignoredFilenames) { + private boolean isSafeToDelete(File[] files) { for (File file : files) { - if (!ignoredFilenames.contains(file.getName())) { + if (Files.isSymbolicLink(file.toPath()) + || file.isDirectory() + || !IGNORED_FILENAMES.contains(file.getName())) { return false; } } @@ -165,10 +258,45 @@ public class FileMoveHelper { } } try { - Files.delete(currentDir); + executeWithRetry(() -> Files.delete(currentDir)); log.info("Deleted empty directory: {}", currentDir); } catch (IOException e) { log.warn("Failed to delete directory: {}", currentDir, e); } } + + @FunctionalInterface + private interface FileOperation { + void execute() throws IOException; + } + + private void executeWithRetry(FileOperation operation) throws IOException { + for (int attempt = 1; attempt <= MAX_ATTEMPTS; attempt++) { + try { + operation.execute(); + return; + } catch (IOException e) { + if (!isRetryableException(e) || attempt == MAX_ATTEMPTS) { + throw e; + } + + log.warn("File operation failed (attempt {}/{}), retrying in {}ms: {}", + attempt, MAX_ATTEMPTS, RETRY_DELAY_MS, e.getMessage()); + + waitBeforeRetry(); + } + } + } + + private boolean isRetryableException(IOException e) { + return RETRYABLE_EXCEPTIONS.stream().anyMatch(type -> type.isInstance(e)); + } + + private void waitBeforeRetry() { + try { + Thread.sleep(RETRY_DELAY_MS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } } diff --git a/booklore-api/src/main/java/com/adityachandel/booklore/service/file/FileMoveService.java b/booklore-api/src/main/java/com/adityachandel/booklore/service/file/FileMoveService.java index 04f711d1f..df604e9d1 100644 --- a/booklore-api/src/main/java/com/adityachandel/booklore/service/file/FileMoveService.java +++ b/booklore-api/src/main/java/com/adityachandel/booklore/service/file/FileMoveService.java @@ -28,6 +28,8 @@ import java.util.stream.Collectors; @Slf4j public class FileMoveService { + private static final long EVENT_DRAIN_TIMEOUT_MS = 300; + private final BookRepository bookRepository; private final LibraryRepository libraryRepository; private final FileMoveHelper fileMoveHelper; @@ -41,79 +43,100 @@ public class FileMoveService { @Transactional public void bulkMoveFiles(FileMoveRequest request) { List moves = request.getMoves(); - Set targetLibraryIds = moves.stream().map(FileMoveRequest.Move::getTargetLibraryId).collect(Collectors.toSet()); - Set sourceLibraryIds = new HashSet<>(); - monitoringRegistrationService.unregisterLibraries(targetLibraryIds); + + Set allAffectedLibraryIds = collectAllAffectedLibraryIds(moves); + Set libraryPaths = monitoringRegistrationService.getPathsForLibraries(allAffectedLibraryIds); + + log.info("Unregistering {} libraries before bulk file move", allAffectedLibraryIds.size()); + monitoringRegistrationService.unregisterLibraries(allAffectedLibraryIds); + monitoringRegistrationService.waitForEventsDrainedByPaths(libraryPaths, EVENT_DRAIN_TIMEOUT_MS); - for (FileMoveRequest.Move move : moves) { - Long bookId = move.getBookId(); - Long targetLibraryId = move.getTargetLibraryId(); - Long targetLibraryPathId = move.getTargetLibraryPathId(); - - Path tempPath = null; - Path currentFilePath = null; - - try { - Optional optionalBook = bookRepository.findById(bookId); - Optional optionalLibrary = libraryRepository.findById(targetLibraryId); - if (optionalBook.isEmpty() || optionalLibrary.isEmpty()) { - continue; - } - BookEntity bookEntity = optionalBook.get(); - LibraryEntity targetLibrary = optionalLibrary.get(); - - Optional optionalLibraryPathEntity = targetLibrary.getLibraryPaths().stream().filter(l -> Objects.equals(l.getId(), targetLibraryPathId)).findFirst(); - if (optionalLibraryPathEntity.isEmpty()) { - continue; - } - LibraryPathEntity libraryPathEntity = optionalLibraryPathEntity.get(); - - LibraryEntity sourceLibrary = bookEntity.getLibrary(); - if (!sourceLibrary.getId().equals(targetLibrary.getId()) && !sourceLibraryIds.contains(sourceLibrary.getId())) { - monitoringRegistrationService.unregisterLibraries(Collections.singleton(sourceLibrary.getId())); - sourceLibraryIds.add(sourceLibrary.getId()); - } - currentFilePath = bookEntity.getFullFilePath(); - String pattern = fileMoveHelper.getFileNamingPattern(targetLibrary); - Path newFilePath = fileMoveHelper.generateNewFilePath(bookEntity, libraryPathEntity, pattern); - if (currentFilePath.equals(newFilePath)) { - continue; - } - - tempPath = fileMoveHelper.moveFileWithBackup(currentFilePath, newFilePath); - - String newFileName = newFilePath.getFileName().toString(); - String newFileSubPath = fileMoveHelper.extractSubPath(newFilePath, libraryPathEntity); - bookRepository.updateFileAndLibrary(bookEntity.getId(), newFileSubPath, newFileName, targetLibrary.getId(), libraryPathEntity); - - fileMoveHelper.commitMove(tempPath, newFilePath); - tempPath = null; - - Path libraryRoot = Paths.get(bookEntity.getLibraryPath().getPath()).toAbsolutePath().normalize(); - fileMoveHelper.deleteEmptyParentDirsUpToLibraryFolders(currentFilePath.getParent(), Set.of(libraryRoot)); - - entityManager.clear(); - - BookEntity fresh = bookRepository.findById(bookId).orElseThrow(); - - notificationService.sendMessage(Topic.BOOK_UPDATE, bookMapper.toBookWithDescription(fresh, false)); - - } catch (Exception e) { - log.error("Error moving file for book ID {}: {}", bookId, e.getMessage(), e); - } finally { - if (tempPath != null && currentFilePath != null) { - fileMoveHelper.rollbackMove(tempPath, currentFilePath); - } + try { + for (FileMoveRequest.Move move : moves) { + processSingleMove(move); + } + } finally { + for (Long libraryId : allAffectedLibraryIds) { + libraryRepository.findById(libraryId) + .ifPresent(library -> monitoringRegistrationService.registerLibrary(libraryMapper.toLibrary(library))); } } + } - for (Long libraryId : targetLibraryIds) { - Optional optionalLibrary = libraryRepository.findById(libraryId); - optionalLibrary.ifPresent(library -> monitoringRegistrationService.registerLibrary(libraryMapper.toLibrary(library))); + private Set collectAllAffectedLibraryIds(List moves) { + Set libraryIds = new HashSet<>(); + + for (FileMoveRequest.Move move : moves) { + libraryIds.add(move.getTargetLibraryId()); + bookRepository.findById(move.getBookId()) + .ifPresent(book -> libraryIds.add(book.getLibrary().getId())); } - for (Long libraryId : sourceLibraryIds) { - Optional optionalLibrary = libraryRepository.findById(libraryId); - optionalLibrary.ifPresent(library -> monitoringRegistrationService.registerLibrary(libraryMapper.toLibrary(library))); + + return libraryIds; + } + + private void processSingleMove(FileMoveRequest.Move move) { + Long bookId = move.getBookId(); + Long targetLibraryId = move.getTargetLibraryId(); + Long targetLibraryPathId = move.getTargetLibraryPathId(); + + Path tempPath = null; + Path currentFilePath = null; + + try { + Optional optionalBook = bookRepository.findById(bookId); + Optional optionalLibrary = libraryRepository.findById(targetLibraryId); + if (optionalBook.isEmpty()) { + log.warn("Book not found for move operation: bookId={}", bookId); + return; + } + if (optionalLibrary.isEmpty()) { + log.warn("Target library not found for move operation: libraryId={}", targetLibraryId); + return; + } + BookEntity bookEntity = optionalBook.get(); + LibraryEntity targetLibrary = optionalLibrary.get(); + + Optional optionalLibraryPathEntity = targetLibrary.getLibraryPaths().stream() + .filter(libraryPath -> Objects.equals(libraryPath.getId(), targetLibraryPathId)) + .findFirst(); + if (optionalLibraryPathEntity.isEmpty()) { + log.warn("Target library path not found for move operation: libraryId={}, pathId={}", targetLibraryId, targetLibraryPathId); + return; + } + LibraryPathEntity libraryPathEntity = optionalLibraryPathEntity.get(); + + currentFilePath = bookEntity.getFullFilePath(); + String pattern = fileMoveHelper.getFileNamingPattern(targetLibrary); + Path newFilePath = fileMoveHelper.generateNewFilePath(bookEntity, libraryPathEntity, pattern); + if (currentFilePath.equals(newFilePath)) { + return; + } + + tempPath = fileMoveHelper.moveFileWithBackup(currentFilePath); + + String newFileName = newFilePath.getFileName().toString(); + String newFileSubPath = fileMoveHelper.extractSubPath(newFilePath, libraryPathEntity); + bookRepository.updateFileAndLibrary(bookEntity.getId(), newFileSubPath, newFileName, targetLibrary.getId(), libraryPathEntity); + + fileMoveHelper.commitMove(tempPath, newFilePath); + tempPath = null; + + Path libraryRoot = Paths.get(bookEntity.getLibraryPath().getPath()).toAbsolutePath().normalize(); + fileMoveHelper.deleteEmptyParentDirsUpToLibraryFolders(currentFilePath.getParent(), Set.of(libraryRoot)); + + entityManager.clear(); + + BookEntity fresh = bookRepository.findById(bookId).orElseThrow(); + + notificationService.sendMessage(Topic.BOOK_UPDATE, bookMapper.toBookWithDescription(fresh, false)); + + } catch (Exception e) { + log.error("Error moving file for book ID {}: {}", bookId, e.getMessage(), e); + } finally { + if (tempPath != null && currentFilePath != null) { + fileMoveHelper.rollbackMove(tempPath, currentFilePath); + } } } @@ -138,7 +161,9 @@ public class FileMoveService { if (isLibraryMonitoredWhenCalled) { log.debug("Unregistering library {} before moving a single file", libraryId); + Set libraryPaths = monitoringRegistrationService.getPathsForLibraries(Set.of(libraryId)); fileMoveHelper.unregisterLibrary(libraryId); + monitoringRegistrationService.waitForEventsDrainedByPaths(libraryPaths, EVENT_DRAIN_TIMEOUT_MS); } fileMoveHelper.moveFile(currentFilePath, expectedFilePath); diff --git a/booklore-api/src/main/java/com/adityachandel/booklore/service/library/FileAsBookProcessor.java b/booklore-api/src/main/java/com/adityachandel/booklore/service/library/FileAsBookProcessor.java index 88ac5bbb8..412391bf3 100644 --- a/booklore-api/src/main/java/com/adityachandel/booklore/service/library/FileAsBookProcessor.java +++ b/booklore-api/src/main/java/com/adityachandel/booklore/service/library/FileAsBookProcessor.java @@ -32,17 +32,21 @@ public class FileAsBookProcessor implements LibraryFileProcessor { @Transactional public void processLibraryFiles(List libraryFiles, LibraryEntity libraryEntity) { for (LibraryFile libraryFile : libraryFiles) { - log.info("Processing file: {}", libraryFile.getFileName()); + processFileWithErrorHandling(libraryFile); + } + log.info("Finished processing library '{}'", libraryEntity.getName()); + } + private void processFileWithErrorHandling(LibraryFile libraryFile) { + log.info("Processing file: {}", libraryFile.getFileName()); + try { FileProcessResult result = processLibraryFile(libraryFile); - if (result != null) { bookEventBroadcaster.broadcastBookAddEvent(result.getBook()); - log.debug("Processed file: {}", libraryFile.getFileName()); } + } catch (Exception e) { + log.error("Failed to process file '{}': {}", libraryFile.getFileName(), e.getMessage()); } - - log.info("Finished processing library '{}'", libraryEntity.getName()); } @Transactional diff --git a/booklore-api/src/main/java/com/adityachandel/booklore/service/monitoring/MonitoringRegistrationService.java b/booklore-api/src/main/java/com/adityachandel/booklore/service/monitoring/MonitoringRegistrationService.java index f4455ef0a..78ca500ea 100644 --- a/booklore-api/src/main/java/com/adityachandel/booklore/service/monitoring/MonitoringRegistrationService.java +++ b/booklore-api/src/main/java/com/adityachandel/booklore/service/monitoring/MonitoringRegistrationService.java @@ -8,7 +8,9 @@ import org.springframework.stereotype.Service; import java.nio.file.Files; import java.nio.file.Path; import java.util.Collection; +import java.util.HashSet; import java.util.Map; +import java.util.Set; @Slf4j @Service @@ -71,4 +73,22 @@ public class MonitoringRegistrationService { } libraryIds.forEach(this::unregisterLibrary); } + + public Set getPathsForLibraries(Collection libraryIds) { + if (libraryIds == null || libraryIds.isEmpty()) { + return Set.of(); + } + return monitoringService.getPathsForLibraries(new HashSet<>(libraryIds)); + } + + public boolean waitForEventsDrainedByPaths(Set paths, long timeoutMs) { + return monitoringService.waitForEventsDrainedByPaths(paths, timeoutMs); + } + + public boolean waitForEventsDrained(Collection libraryIds, long timeoutMs) { + if (libraryIds == null || libraryIds.isEmpty()) { + return true; + } + return monitoringService.waitForEventsDrained(new HashSet<>(libraryIds), timeoutMs); + } } \ No newline at end of file diff --git a/booklore-api/src/main/java/com/adityachandel/booklore/service/monitoring/MonitoringService.java b/booklore-api/src/main/java/com/adityachandel/booklore/service/monitoring/MonitoringService.java index acde6d8f9..e415d9562 100644 --- a/booklore-api/src/main/java/com/adityachandel/booklore/service/monitoring/MonitoringService.java +++ b/booklore-api/src/main/java/com/adityachandel/booklore/service/monitoring/MonitoringService.java @@ -32,7 +32,6 @@ public class MonitoringService { private final Map registeredWatchKeys = new ConcurrentHashMap<>(); private final Map pathToLibraryIdMap = new ConcurrentHashMap<>(); private final Map libraryWatchStatusMap = new ConcurrentHashMap<>(); - private final Map> libraryIdToPaths = new ConcurrentHashMap<>(); public MonitoringService(LibraryFileEventProcessor libraryFileEventProcessor, WatchService watchService, MonitoringTask monitoringTask) { this.libraryFileEventProcessor = libraryFileEventProcessor; @@ -67,7 +66,6 @@ public class MonitoringService { libraryWatchStatusMap.put(library.getId(), library.isWatch()); if (!library.isWatch()) return; - List registeredPaths = new ArrayList<>(); int[] registeredCount = {0}; library.getPaths().forEach(libraryPath -> { @@ -77,7 +75,6 @@ public class MonitoringService { pathStream.filter(Files::isDirectory).forEach(path -> { if (registerPath(path, library.getId())) { registeredCount[0]++; - registeredPaths.add(path); } }); } catch (IOException e) { @@ -86,7 +83,6 @@ public class MonitoringService { } }); - libraryIdToPaths.put(library.getId(), registeredPaths); log.info("Registered {} folders for library '{}'", registeredCount[0], library.getName()); } @@ -101,7 +97,6 @@ public class MonitoringService { } libraryWatchStatusMap.put(libraryId, false); - libraryIdToPaths.remove(libraryId); log.debug("Unregistered library {} from monitoring", libraryId); } @@ -245,4 +240,51 @@ public class MonitoringService { public boolean isLibraryMonitored(Long libraryId) { return libraryWatchStatusMap.getOrDefault(libraryId, false); } + + public Set getPathsForLibraries(Set libraryIds) { + return pathToLibraryIdMap.entrySet().stream() + .filter(entry -> libraryIds.contains(entry.getValue())) + .map(Map.Entry::getKey) + .collect(Collectors.toSet()); + } + + public boolean waitForEventsDrained(Set libraryIds, long timeoutMs) { + if (libraryIds == null || libraryIds.isEmpty()) { + return true; + } + + Set libraryPaths = getPathsForLibraries(libraryIds); + return waitForEventsDrainedByPaths(libraryPaths, timeoutMs); + } + + public boolean waitForEventsDrainedByPaths(Set libraryPaths, long timeoutMs) { + if (libraryPaths == null || libraryPaths.isEmpty()) { + return true; + } + + final long pollIntervalMs = 50; + long deadline = System.currentTimeMillis() + timeoutMs; + while (System.currentTimeMillis() < deadline) { + boolean hasPendingEvents = eventQueue.stream() + .anyMatch(event -> { + Path watchedFolder = event.getWatchedFolder(); + return libraryPaths.stream().anyMatch(watchedFolder::startsWith); + }); + + if (!hasPendingEvents) { + log.debug("Event queue drained for paths: {}", libraryPaths.size()); + return true; + } + + try { + Thread.sleep(pollIntervalMs); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + return false; + } + } + + log.warn("Timeout waiting for event queue to drain for {} paths", libraryPaths.size()); + return false; + } } diff --git a/booklore-api/src/test/java/com/adityachandel/booklore/service/file/FileMoveHelperTest.java b/booklore-api/src/test/java/com/adityachandel/booklore/service/file/FileMoveHelperTest.java new file mode 100644 index 000000000..f3f17e954 --- /dev/null +++ b/booklore-api/src/test/java/com/adityachandel/booklore/service/file/FileMoveHelperTest.java @@ -0,0 +1,365 @@ +package com.adityachandel.booklore.service.file; + +import com.adityachandel.booklore.service.appsettings.AppSettingService; +import com.adityachandel.booklore.service.monitoring.MonitoringRegistrationService; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.io.TempDir; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.NoSuchFileException; +import java.nio.file.Path; +import java.util.Set; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; + +import static org.junit.jupiter.api.Assertions.*; + +@ExtendWith(MockitoExtension.class) +@DisplayName("FileMoveHelper Tests") +class FileMoveHelperTest { + + @Mock + private MonitoringRegistrationService monitoringRegistrationService; + + @Mock + private AppSettingService appSettingService; + + private FileMoveHelper fileMoveHelper; + + @TempDir + Path tempDir; + + @BeforeEach + void setUp() { + fileMoveHelper = new FileMoveHelper(monitoringRegistrationService, appSettingService); + } + + @Nested + @DisplayName("SMB Share Behavior Tests") + class SmbShareBehaviorTests { + + @Test + @DisplayName("waitForFileAccessible retries when file appears after delay (simulates SMB latency)") + void waitForFileAccessible_fileAppearsAfterDelay_eventuallySucceeds() throws Exception { + Path filePath = tempDir.resolve("delayed-file.txt"); + + ScheduledExecutorService executor = Executors.newSingleThreadScheduledExecutor(); + try { + executor.schedule(() -> { + try { + Files.writeString(filePath, "content"); + } catch (IOException e) { + throw new RuntimeException(e); + } + }, 150, TimeUnit.MILLISECONDS); + + boolean result = fileMoveHelper.waitForFileAccessible(filePath); + + assertTrue(result, "File should become accessible after retries"); + assertTrue(Files.exists(filePath)); + } finally { + executor.shutdownNow(); + } + } + + @Test + @DisplayName("waitForFileAccessible returns false when file never appears (simulates SMB file not found)") + void waitForFileAccessible_fileNeverAppears_returnsFalse() { + Path nonExistentFile = tempDir.resolve("does-not-exist.txt"); + + long startTime = System.currentTimeMillis(); + boolean result = fileMoveHelper.waitForFileAccessible(nonExistentFile); + long elapsed = System.currentTimeMillis() - startTime; + + assertFalse(result); + assertTrue(elapsed >= 200, "Should have waited for at least 2 retry delays (100ms each)"); + } + + @Test + @DisplayName("moveFile throws NoSuchFileException when source never becomes accessible") + void moveFile_sourceNeverAccessible_throwsNoSuchFileException() { + Path source = tempDir.resolve("missing-source.txt"); + Path target = tempDir.resolve("target.txt"); + + NoSuchFileException exception = assertThrows(NoSuchFileException.class, + () -> fileMoveHelper.moveFile(source, target)); + + assertTrue(exception.getMessage().contains("Source file not accessible after retries")); + } + + @Test + @DisplayName("moveFile succeeds when source appears during retry (simulates SMB eventual consistency)") + void moveFile_sourceAppearsAfterRetry_succeeds() throws Exception { + Path source = tempDir.resolve("eventual-source.txt"); + Path target = tempDir.resolve("subdir/target.txt"); + + ScheduledExecutorService executor = Executors.newSingleThreadScheduledExecutor(); + try { + executor.schedule(() -> { + try { + Files.writeString(source, "delayed content"); + } catch (IOException e) { + throw new RuntimeException(e); + } + }, 50, TimeUnit.MILLISECONDS); + + fileMoveHelper.moveFile(source, target); + + assertTrue(Files.exists(target)); + assertFalse(Files.exists(source)); + assertEquals("delayed content", Files.readString(target)); + } finally { + executor.shutdownNow(); + } + } + + @Test + @DisplayName("moveFileWithBackup creates temp file and returns its path") + void moveFileWithBackup_createsTemporaryFile() throws Exception { + Path source = tempDir.resolve("original.txt"); + Files.writeString(source, "original content"); + + Path tempPath = fileMoveHelper.moveFileWithBackup(source); + + assertFalse(Files.exists(source), "Original should be moved"); + assertTrue(Files.exists(tempPath), "Temp file should exist"); + assertTrue(tempPath.getFileName().toString().endsWith(".tmp_move")); + assertEquals("original content", Files.readString(tempPath)); + } + + @Test + @DisplayName("commitMove moves temp file to final destination") + void commitMove_movesToFinalDestination() throws Exception { + Path tempPath = tempDir.resolve("file.txt.tmp_move"); + Path target = tempDir.resolve("subdir/final.txt"); + Files.writeString(tempPath, "content"); + + fileMoveHelper.commitMove(tempPath, target); + + assertFalse(Files.exists(tempPath), "Temp file should be removed"); + assertTrue(Files.exists(target), "Target should exist"); + assertEquals("content", Files.readString(target)); + } + + @Test + @DisplayName("rollbackMove restores original file from temp location") + void rollbackMove_restoresOriginalFile() throws Exception { + Path tempPath = tempDir.resolve("file.txt.tmp_move"); + Path originalSource = tempDir.resolve("original.txt"); + Files.writeString(tempPath, "content"); + + fileMoveHelper.rollbackMove(tempPath, originalSource); + + assertFalse(Files.exists(tempPath), "Temp file should be removed"); + assertTrue(Files.exists(originalSource), "Original should be restored"); + assertEquals("content", Files.readString(originalSource)); + } + + @Test + @DisplayName("rollbackMove does nothing when temp file doesn't exist") + void rollbackMove_noTempFile_doesNothing() { + Path tempPath = tempDir.resolve("nonexistent.tmp_move"); + Path originalSource = tempDir.resolve("original.txt"); + + assertDoesNotThrow(() -> fileMoveHelper.rollbackMove(tempPath, originalSource)); + assertFalse(Files.exists(originalSource)); + } + } + + @Nested + @DisplayName("Retry Mechanism Tests") + class RetryMechanismTests { + + @Test + @DisplayName("waitForFileAccessible succeeds immediately when file exists") + void waitForFileAccessible_fileExists_succeedsImmediately() throws Exception { + Path existingFile = tempDir.resolve("existing.txt"); + Files.writeString(existingFile, "content"); + + long startTime = System.currentTimeMillis(); + boolean result = fileMoveHelper.waitForFileAccessible(existingFile); + long elapsed = System.currentTimeMillis() - startTime; + + assertTrue(result); + assertTrue(elapsed < 50, "Should return immediately without delays"); + } + } + + @Nested + @DisplayName("Directory Cleanup Tests") + class DirectoryCleanupTests { + + @Test + @DisplayName("Deletes empty parent directories up to library root") + void deleteEmptyParentDirs_deletesEmptyDirectories() throws Exception { + Path libraryRoot = tempDir.resolve("library"); + Path nestedDir = libraryRoot.resolve("author/series/book"); + Files.createDirectories(nestedDir); + + fileMoveHelper.deleteEmptyParentDirsUpToLibraryFolders(nestedDir, Set.of(libraryRoot)); + + assertFalse(Files.exists(nestedDir)); + assertFalse(Files.exists(libraryRoot.resolve("author/series"))); + assertFalse(Files.exists(libraryRoot.resolve("author"))); + assertTrue(Files.exists(libraryRoot), "Library root should not be deleted"); + } + + @Test + @DisplayName("Stops at directory containing non-ignored files") + void deleteEmptyParentDirs_stopsAtNonEmptyDirectory() throws Exception { + Path libraryRoot = tempDir.resolve("library"); + Path authorDir = libraryRoot.resolve("author"); + Path seriesDir = authorDir.resolve("series"); + Path bookDir = seriesDir.resolve("book"); + Files.createDirectories(bookDir); + Files.writeString(authorDir.resolve("other-file.txt"), "content"); + + fileMoveHelper.deleteEmptyParentDirsUpToLibraryFolders(bookDir, Set.of(libraryRoot)); + + assertFalse(Files.exists(bookDir)); + assertFalse(Files.exists(seriesDir)); + assertTrue(Files.exists(authorDir), "Should stop at directory with other files"); + assertTrue(Files.exists(authorDir.resolve("other-file.txt"))); + } + + @Test + @DisplayName("Deletes directories containing only ignored files (.DS_Store, Thumbs.db)") + void deleteEmptyParentDirs_handlesMultipleIgnoredFiles() throws Exception { + Path libraryRoot = tempDir.resolve("library"); + Path nestedDir = libraryRoot.resolve("author"); + Files.createDirectories(nestedDir); + Files.writeString(nestedDir.resolve(".DS_Store"), "mac metadata"); + Files.writeString(nestedDir.resolve("Thumbs.db"), "windows thumbnail cache"); + + fileMoveHelper.deleteEmptyParentDirsUpToLibraryFolders(nestedDir, Set.of(libraryRoot)); + + assertFalse(Files.exists(nestedDir)); + assertTrue(Files.exists(libraryRoot)); + } + + @Test + @DisplayName("Does not delete library root even if empty") + void deleteEmptyParentDirs_neverDeletesLibraryRoot() throws Exception { + Path libraryRoot = tempDir.resolve("library"); + Files.createDirectories(libraryRoot); + + fileMoveHelper.deleteEmptyParentDirsUpToLibraryFolders(libraryRoot, Set.of(libraryRoot)); + + assertTrue(Files.exists(libraryRoot), "Library root should never be deleted"); + } + + @Test + @DisplayName("Recursively deletes nested empty subdirectories") + void deleteEmptyParentDirs_deletesNestedEmptySubdirectories() throws Exception { + Path libraryRoot = tempDir.resolve("library"); + Path authorDir = libraryRoot.resolve("author"); + Path seriesDir = authorDir.resolve("series"); + Path deepEmptyDir = seriesDir.resolve("volume/chapter"); + Files.createDirectories(deepEmptyDir); + + fileMoveHelper.deleteEmptyParentDirsUpToLibraryFolders(authorDir, Set.of(libraryRoot)); + + assertFalse(Files.exists(deepEmptyDir), "Deep empty directory should be deleted"); + assertFalse(Files.exists(seriesDir), "Series directory should be deleted"); + assertFalse(Files.exists(authorDir), "Author directory should be deleted"); + assertTrue(Files.exists(libraryRoot), "Library root should remain"); + } + + @Test + @DisplayName("Does not delete subdirectories containing real files") + void deleteEmptyParentDirs_preservesSubdirectoriesWithFiles() throws Exception { + Path libraryRoot = tempDir.resolve("library"); + Path authorDir = libraryRoot.resolve("author"); + Path seriesWithBook = authorDir.resolve("series-with-book"); + Path emptySeriesDir = authorDir.resolve("empty-series"); + Files.createDirectories(seriesWithBook); + Files.createDirectories(emptySeriesDir); + Files.writeString(seriesWithBook.resolve("book.epub"), "ebook content"); + + fileMoveHelper.deleteEmptyParentDirsUpToLibraryFolders(authorDir, Set.of(libraryRoot)); + + assertFalse(Files.exists(emptySeriesDir), "Empty subdirectory should be deleted"); + assertTrue(Files.exists(seriesWithBook), "Subdirectory with files should remain"); + assertTrue(Files.exists(seriesWithBook.resolve("book.epub")), "File should remain"); + assertTrue(Files.exists(authorDir), "Parent should remain because it has non-empty subdirectory"); + } + + @Test + @DisplayName("Deletes subdirectories containing only ignored files") + void deleteEmptyParentDirs_deletesSubdirectoriesWithOnlyIgnoredFiles() throws Exception { + Path libraryRoot = tempDir.resolve("library"); + Path authorDir = libraryRoot.resolve("author"); + Path subDirWithIgnored = authorDir.resolve("series"); + Files.createDirectories(subDirWithIgnored); + Files.writeString(subDirWithIgnored.resolve(".DS_Store"), "mac metadata"); + Files.writeString(subDirWithIgnored.resolve("Thumbs.db"), "windows cache"); + + fileMoveHelper.deleteEmptyParentDirsUpToLibraryFolders(authorDir, Set.of(libraryRoot)); + + assertFalse(Files.exists(subDirWithIgnored), "Subdirectory with only ignored files should be deleted"); + assertFalse(Files.exists(authorDir), "Parent should be deleted"); + assertTrue(Files.exists(libraryRoot), "Library root should remain"); + } + } + + @Nested + @DisplayName("Edge Cases") + class EdgeCaseTests { + + @Test + @DisplayName("moveFile creates target directory if it doesn't exist") + void moveFile_createsTargetDirectory() throws Exception { + Path source = tempDir.resolve("source.txt"); + Path target = tempDir.resolve("deep/nested/directory/target.txt"); + Files.writeString(source, "content"); + + fileMoveHelper.moveFile(source, target); + + assertTrue(Files.exists(target)); + assertTrue(Files.isDirectory(target.getParent())); + } + + @Test + @DisplayName("moveFile handles file with special characters in name") + void moveFile_specialCharactersInName() throws Exception { + Path source = tempDir.resolve("file with spaces & special (chars).txt"); + Path target = tempDir.resolve("renamed [file].txt"); + Files.writeString(source, "content"); + + fileMoveHelper.moveFile(source, target); + + assertTrue(Files.exists(target)); + assertEquals("content", Files.readString(target)); + } + + @Test + @DisplayName("deleteEmptyParentDirs handles symlinks safely") + void deleteEmptyParentDirs_handlesSymlinks() throws Exception { + Path libraryRoot = tempDir.resolve("library"); + Path realDir = tempDir.resolve("realdir"); + Path nestedDir = libraryRoot.resolve("nested"); + Files.createDirectories(realDir); + Files.createDirectories(nestedDir); + + try { + Path symlink = nestedDir.resolve("symlink"); + Files.createSymbolicLink(symlink, realDir); + + fileMoveHelper.deleteEmptyParentDirsUpToLibraryFolders(nestedDir, Set.of(libraryRoot)); + + assertTrue(Files.exists(nestedDir), "Should not delete dir with symlink"); + assertTrue(Files.exists(realDir), "Real directory should still exist"); + } catch (UnsupportedOperationException e) { + // Symlinks not supported on this system, skip test + } + } + } +}