Fix handling of Windows junctions during temp dir cleanup (#4301)

* Stop following working junctions (same as symlinks)
* Fix deleting broken junctions

Fixes #4299.

Author: marcphilipp

documentation/src/docs/asciidoc/release-notes/release-notes-5.12.0-RC1.adoc

@@ -36,7 +36,8 @@ repository on GitHub.
 [[release-notes-5.12.0-RC1-junit-jupiter-bug-fixes]]
 ==== Bug Fixes
 
-* ❓
+* Fix handling of "junctions" on Windows during `@TempDir` cleanup: junctions will no
+  longer be followed when deleting directories and broken junctions will be deleted.
 
 [[release-notes-5.12.0-RC1-junit-jupiter-deprecations-and-breaking-changes]]
 ==== Deprecations and Breaking Changes

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TempDirectory.java

@@ -11,6 +11,7 @@
 package org.junit.jupiter.engine.extension;
 
 import static java.nio.file.FileVisitResult.CONTINUE;
+import static java.nio.file.FileVisitResult.SKIP_SUBTREE;
 import static java.util.stream.Collectors.joining;
 import static org.junit.jupiter.api.extension.TestInstantiationAwareExtension.ExtensionContextScope.TEST_METHOD;
 import static org.junit.jupiter.api.io.CleanupMode.DEFAULT;
@@ -32,6 +33,7 @@
 import java.nio.file.FileSystems;
 import java.nio.file.FileVisitResult;
 import java.nio.file.Files;
+import java.nio.file.LinkOption;
 import java.nio.file.NoSuchFileException;
 import java.nio.file.Path;
 import java.nio.file.Paths;
@@ -289,7 +291,7 @@ private static boolean selfOrChildFailed(ExtensionContext context) {
 
 	static class CloseablePath implements CloseableResource {
 
-		private static final Logger logger = LoggerFactory.getLogger(CloseablePath.class);
+		private static final Logger LOGGER = LoggerFactory.getLogger(CloseablePath.class);
 
 		private final Path dir;
 		private final TempDirFactory factory;
@@ -327,15 +329,27 @@ public void close() throws IOException {
 			try {
 				if (this.cleanupMode == NEVER
 						|| (this.cleanupMode == ON_SUCCESS && selfOrChildFailed(this.extensionContext))) {
-					logger.info(() -> String.format("Skipping cleanup of temp dir %s for %s due to CleanupMode.%s.",
+					LOGGER.info(() -> String.format("Skipping cleanup of temp dir %s for %s due to CleanupMode.%s.",
 						this.dir, descriptionFor(this.annotatedElement), this.cleanupMode.name()));
 					return;
 				}
 
 				FileOperations fileOperations = this.extensionContext.getStore(NAMESPACE) //
 						.getOrDefault(FILE_OPERATIONS_KEY, FileOperations.class, FileOperations.DEFAULT);
+				FileOperations loggingFileOperations = file -> {
+					LOGGER.trace(() -> "Attempting to delete " + file);
+					try {
+						fileOperations.delete(file);
+						LOGGER.trace(() -> "Successfully deleted " + file);
+					}
+					catch (IOException e) {
+						LOGGER.trace(e, () -> "Failed to delete " + file);
+						throw e;
+					}
+				};
 
-				SortedMap<Path, IOException> failures = deleteAllFilesAndDirectories(fileOperations);
+				LOGGER.trace(() -> "Cleaning up temp dir " + this.dir);
+				SortedMap<Path, IOException> failures = deleteAllFilesAndDirectories(loggingFileOperations);
 				if (!failures.isEmpty()) {
 					throw createIOExceptionWithAttachedFailures(failures);
 				}
@@ -375,26 +389,41 @@ private static String descriptionFor(Executable executable) {
 		private SortedMap<Path, IOException> deleteAllFilesAndDirectories(FileOperations fileOperations)
 				throws IOException {
 
-			if (this.dir == null || Files.notExists(this.dir)) {
+			Path rootDir = this.dir;
+			if (rootDir == null || Files.notExists(rootDir)) {
 				return Collections.emptySortedMap();
 			}
 
 			SortedMap<Path, IOException> failures = new TreeMap<>();
 			Set<Path> retriedPaths = new HashSet<>();
-			tryToResetPermissions(this.dir);
-			Files.walkFileTree(this.dir, new SimpleFileVisitor<Path>() {
+			Path rootRealPath = rootDir.toRealPath();
+
+			tryToResetPermissions(rootDir);
+			Files.walkFileTree(rootDir, new SimpleFileVisitor<Path>() {
 
 				@Override
-				public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) {
-					if (!dir.equals(CloseablePath.this.dir)) {
+				public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException {
+					LOGGER.trace(() -> "preVisitDirectory: " + dir);
+					if (isLink(dir)) {
+						delete(dir);
+						return SKIP_SUBTREE;
+					}
+					if (!dir.equals(rootDir)) {
 						tryToResetPermissions(dir);
 					}
 					return CONTINUE;
 				}
 
+				private boolean isLink(Path dir) throws IOException {
+					// While `Files.walkFileTree` does not follow symbolic links, it may follow other links
+					// such as "junctions" on Windows
+					return !dir.toRealPath().startsWith(rootRealPath);
+				}
+
 				@Override
 				public FileVisitResult visitFileFailed(Path file, IOException exc) {
-					if (exc instanceof NoSuchFileException) {
+					LOGGER.trace(exc, () -> "visitFileFailed: " + file);
+					if (exc instanceof NoSuchFileException && !Files.exists(file, LinkOption.NOFOLLOW_LINKS)) {
 						return CONTINUE;
 					}
 					// IOException includes `AccessDeniedException` thrown by non-readable or non-executable flags
@@ -404,15 +433,19 @@ public FileVisitResult visitFileFailed(Path file, IOException exc) {
 
 				@Override
 				public FileVisitResult visitFile(Path file, BasicFileAttributes attributes) {
-					return deleteAndContinue(file);
+					LOGGER.trace(() -> "visitFile: " + file);
+					delete(file);
+					return CONTINUE;
 				}
 
 				@Override
 				public FileVisitResult postVisitDirectory(Path dir, IOException exc) {
-					return deleteAndContinue(dir);
+					LOGGER.trace(exc, () -> "postVisitDirectory: " + dir);
+					delete(dir);
+					return CONTINUE;
 				}
 
-				private FileVisitResult deleteAndContinue(Path path) {
+				private void delete(Path path) {
 					try {
 						fileOperations.delete(path);
 					}
@@ -426,7 +459,6 @@ private FileVisitResult deleteAndContinue(Path path) {
 						// IOException includes `AccessDeniedException` thrown by non-readable or non-executable flags
 						resetPermissionsAndTryToDeleteAgain(path, exception);
 					}
-					return CONTINUE;
 				}
 
 				private void resetPermissionsAndTryToDeleteAgain(Path path, IOException exception) {

jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/TempDirectoryCleanupTests.java

@@ -13,6 +13,7 @@
 import static java.nio.file.Files.deleteIfExists;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.fail;
+import static org.junit.jupiter.api.condition.OS.WINDOWS;
 import static org.junit.jupiter.api.io.CleanupMode.ALWAYS;
 import static org.junit.jupiter.api.io.CleanupMode.NEVER;
 import static org.junit.jupiter.api.io.CleanupMode.ON_SUCCESS;
@@ -21,6 +22,7 @@
 import static org.junit.platform.launcher.core.LauncherDiscoveryRequestBuilder.request;
 
 import java.io.IOException;
+import java.nio.file.Files;
 import java.nio.file.Path;
 
 import org.junit.jupiter.api.AfterAll;
@@ -29,6 +31,7 @@
 import org.junit.jupiter.api.Order;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.TestMethodOrder;
+import org.junit.jupiter.api.condition.EnabledOnOs;
 import org.junit.jupiter.api.io.CleanupMode;
 import org.junit.jupiter.api.io.TempDir;
 import org.junit.jupiter.engine.AbstractJupiterTestEngineTests;
@@ -452,4 +455,57 @@ void testOnSuccessFailingParameter(@TempDir(cleanup = ON_SUCCESS) Path onSuccess
 
 	}
 
+	@Nested
+	@EnabledOnOs(WINDOWS)
+	class WindowsTests {
+
+		@Test
+		void deletesBrokenJunctions(@TempDir Path dir) throws Exception {
+			var test = Files.createDirectory(dir.resolve("test"));
+			createWindowsJunction(dir.resolve("link"), test);
+			// The error might also occur without the source folder being deleted
+			// but it depends on the order that the TempDir cleanup does its work,
+			// so the following line forces the error to occur always
+			Files.delete(test);
+		}
+
+		@Test
+		void doesNotFollowJunctions(@TempDir Path tempDir) throws IOException {
+			var outsideDir = Files.createDirectory(tempDir.resolve("outside"));
+			var testFile = Files.writeString(outsideDir.resolve("test.txt"), "test");
+
+			JunctionTestCase.target = outsideDir;
+			try {
+				executeTestsForClass(JunctionTestCase.class).testEvents() //
+						.assertStatistics(stats -> stats.started(1).succeeded(1));
+			}
+			finally {
+				JunctionTestCase.target = null;
+			}
+
+			assertThat(outsideDir).exists();
+			assertThat(testFile).exists();
+		}
+
+		@SuppressWarnings("JUnitMalformedDeclaration")
+		static class JunctionTestCase {
+			public static Path target;
+
+			@Test
+			void createJunctionToTarget(@TempDir Path dir) throws Exception {
+				var link = createWindowsJunction(dir.resolve("link"), target);
+				try (var files = Files.list(link)) {
+					files.forEach(it -> System.out.println("- " + it));
+				}
+			}
+		}
+
+		private static Path createWindowsJunction(Path link, Path target) throws Exception {
+			// This creates a Windows "junction" which you can't do with Java code
+			String[] command = { "cmd.exe", "/c", "mklink", "/j", link.toString(), target.toString() };
+			Runtime.getRuntime().exec(command).waitFor();
+			return link;
+		}
+	}
+
 }

jupiter-tests/src/test/resources/junit-platform.properties

@@ -1 +1,4 @@
 junit.jupiter.extensions.autodetection.enabled=true
+
+junit.platform.output.capture.stdout=true
+junit.platform.output.capture.stderr=true

jupiter-tests/src/test/resources/log4j2-test.xml

@@ -11,8 +11,9 @@
 		<Logger name="org.junit.jupiter.engine" level="off" />
 		<Logger name="org.junit.platform.launcher.listeners.discovery.LoggingLauncherDiscoveryListener" level="off" />
 		<Logger name="org.junit.jupiter.engine.extension.MutableExtensionRegistry" level="off" />
+		<Logger name="org.junit.jupiter.engine.extension.TempDirectory$CloseablePath" level="off" />
 		<Root level="error">
 			<AppenderRef ref="Console" />
 		</Root>
 	</Loggers>
-</Configuration>
+</Configuration>