Merge pull request #1745 from hapifhir/do-20240909-package-lock-cleanup

Package Cache lock cleanup

org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/npm/FilesystemPackageCacheManager.java

@@ -8,6 +8,7 @@ import java.text.SimpleDateFormat;
 import java.util.*;
 
 import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
 
 import lombok.Getter;
 import lombok.Setter;
@@ -77,12 +78,13 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple
 
   private final FilesystemPackageCacheManagerLocks locks;
 
+  private final FilesystemPackageCacheManagerLocks.LockParameters lockParameters;
+
   // When running in testing mode, some packages are provided from the test case repository rather than by the normal means
   // the PackageProvider is responsible for this. if no package provider is defined, or it declines to handle the package, 
   // then the normal means will be used
   public interface IPackageProvider {
     boolean handlesPackage(String id, String version);
-
     InputStreamWithSrc provide(String id, String version) throws IOException;
   }
 
@@ -92,6 +94,7 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple
   public static final String PACKAGE_VERSION_REGEX_OPT = "^[A-Za-z][A-Za-z0-9\\_\\-]*(\\.[A-Za-z0-9\\_\\-]+)+(\\#[A-Za-z0-9\\-\\_]+(\\.[A-Za-z0-9\\-\\_]+)*)?$";
   private static final Logger ourLog = LoggerFactory.getLogger(FilesystemPackageCacheManager.class);
   private static final String CACHE_VERSION = "3"; // second version - see wiki page
+
   @Nonnull
   private final File cacheFolder;
 
@@ -100,6 +103,7 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple
   private final Map<String, String> ciList = new HashMap<>();
   private JsonArray buildInfo;
   private boolean suppressErrors;
+
   @Setter
   @Getter
   private boolean minimalMemory;
@@ -113,9 +117,20 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple
     @Getter
     private final List<PackageServer> packageServers;
 
+    @With
+    @Getter
+    private final FilesystemPackageCacheManagerLocks.LockParameters lockParameters;
+
     public Builder() throws IOException {
       this.cacheFolder = getUserCacheFolder();
       this.packageServers = getPackageServersFromFHIRSettings();
+      this.lockParameters = null;
+    }
+
+    private Builder(File cacheFolder, List<PackageServer> packageServers, FilesystemPackageCacheManagerLocks.LockParameters lockParameters) {
+      this.cacheFolder = cacheFolder;
+      this.packageServers = packageServers;
+      this.lockParameters = lockParameters;
     }
 
     private File getUserCacheFolder() throws IOException {
@@ -143,17 +158,12 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple
       return PackageServer.getConfiguredServers();
     }
 
-    private Builder(File cacheFolder, List<PackageServer> packageServers) {
-      this.cacheFolder = cacheFolder;
-      this.packageServers = packageServers;
-    }
-
     public Builder withCacheFolder(String cacheFolderPath) throws IOException {
       File cacheFolder = ManagedFileAccess.file(cacheFolderPath);
       if (!cacheFolder.exists()) {
         throw new FHIRException("The folder '" + cacheFolder + "' could not be found");
       }
-      return new Builder(cacheFolder, this.packageServers);
+      return new Builder(cacheFolder, this.packageServers, this.lockParameters);
     }
 
     public Builder withSystemCacheFolder() throws IOException {
@@ -163,32 +173,33 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple
       } else {
         systemCacheFolder = ManagedFileAccess.file(Utilities.path("/var", "lib", ".fhir", "packages"));
       }
-      return new Builder(systemCacheFolder, this.packageServers);
+      return new Builder(systemCacheFolder, this.packageServers, this.lockParameters);
     }
 
     public Builder withTestingCacheFolder() throws IOException {
-      return new Builder(ManagedFileAccess.file(Utilities.path("[tmp]", ".fhir", "packages")), this.packageServers);
+      return new Builder(ManagedFileAccess.file(Utilities.path("[tmp]", ".fhir", "packages")), this.packageServers, this.lockParameters);
     }
 
     public FilesystemPackageCacheManager build() throws IOException {
-      return new FilesystemPackageCacheManager(cacheFolder, packageServers);
+      final FilesystemPackageCacheManagerLocks locks;
+      try {
+        locks = FilesystemPackageCacheManagerLocks.getFilesystemPackageCacheManagerLocks(cacheFolder);
+      } catch (RuntimeException e) {
+        if (e.getCause() instanceof IOException) {
+          throw (IOException) e.getCause();
+        } else {
+          throw e;
+        }
+      }
+      return new FilesystemPackageCacheManager(cacheFolder, packageServers, locks, lockParameters);
     }
   }
 
-  private FilesystemPackageCacheManager(@Nonnull File cacheFolder, @Nonnull List<PackageServer> packageServers) throws IOException {
+  private FilesystemPackageCacheManager(@Nonnull File cacheFolder, @Nonnull List<PackageServer> packageServers, @Nonnull FilesystemPackageCacheManagerLocks locks, @Nullable FilesystemPackageCacheManagerLocks.LockParameters lockParameters) throws IOException {
     super(packageServers);
     this.cacheFolder = cacheFolder;
-
+    this.locks = locks;
-    try {
+    this.lockParameters = lockParameters;
-      this.locks = FilesystemPackageCacheManagerLocks.getFilesystemPackageCacheManagerLocks(cacheFolder);
-    } catch (RuntimeException e) {
-      if (e.getCause() instanceof IOException) {
-        throw (IOException) e.getCause();
-      } else {
-        throw e;
-      }
-    }
-
     prepareCacheFolder();
   }
 
@@ -218,11 +229,35 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple
           createIniFile();
         }
         deleteOldTempDirectories();
+        cleanUpCorruptPackages();
       }
       return null;
     });
   }
 
+  /*
+    Look for .lock files that are not actively held by a process. If found, delete the lock file, and the package
+    referenced.
+   */
+  protected void cleanUpCorruptPackages() throws IOException {
+    for (File file : Objects.requireNonNull(cacheFolder.listFiles())) {
+      if (file.getName().endsWith(".lock")) {
+        if (locks.getCacheLock().canLockFileBeHeldByThisProcess(file)) {
+          String packageDirectoryName = file.getName().substring(0, file.getName().length() - 5);
+          log("Detected potential incomplete package installed in cache: " + packageDirectoryName + ". Attempting to delete");
+
+          File packageDirectory = ManagedFileAccess.file(Utilities.path(cacheFolder, packageDirectoryName));
+          if (packageDirectory.exists()) {
+            Utilities.clearDirectory(packageDirectory.getAbsolutePath());
+            packageDirectory.delete();
+          }
+          file.delete();
+          log("Deleted potential incomplete package: " + packageDirectoryName);
+        }
+      }
+    }
+  }
+
   private boolean iniFileExists() throws IOException {
     String iniPath = getPackagesIniPath();
     File iniFile = ManagedFileAccess.file(iniPath);
@@ -421,7 +456,7 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple
       }
 
       return null;
-    });
+    }, lockParameters);
   }
 
   /**
@@ -465,7 +500,7 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple
           return null;
         }
         return loadPackageInfo(path);
-      });
+      }, lockParameters);
       if (foundPackage != null) {
         if (foundPackage.isIndexed()){
           return foundPackage;
@@ -488,7 +523,7 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple
             String path = Utilities.path(cacheFolder, foundPackageFolder);
             output.checkIndexed(path);
             return output;
-          });
+          }, lockParameters);
           }
       }
     }
@@ -589,7 +624,7 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple
         throw e;
       }
       return npmPackage;
-    });
+    }, lockParameters);
   }
 
   private void log(String s) {

org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/npm/FilesystemPackageCacheManagerLocks.java

@@ -1,14 +1,19 @@
 package org.hl7.fhir.utilities.npm;
 
 import lombok.Getter;
-import org.hl7.fhir.utilities.TextFile;
+import lombok.With;
 import org.hl7.fhir.utilities.Utilities;
+import org.hl7.fhir.utilities.filesystem.ManagedFileAccess;
 
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
 import java.io.File;
 import java.io.IOException;
 import java.io.RandomAccessFile;
+import java.nio.ByteBuffer;
 import java.nio.channels.FileChannel;
 import java.nio.channels.FileLock;
+import java.nio.charset.StandardCharsets;
 import java.nio.file.*;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.TimeUnit;
@@ -27,9 +32,23 @@ public class FilesystemPackageCacheManagerLocks {
 
   private final File cacheFolder;
 
-  private final Long lockTimeoutTime;
+  private static final LockParameters lockParameters = new LockParameters();
 
-  private final TimeUnit lockTimeoutTimeUnit;
+  public static class LockParameters {
+    @Getter @With
+    private final long lockTimeoutTime;
+    @Getter @With
+    private final TimeUnit lockTimeoutTimeUnit;
+
+    public LockParameters() {
+      this(60L, TimeUnit.SECONDS);
+    }
+
+    public LockParameters(long lockTimeoutTime, TimeUnit lockTimeoutTimeUnit) {
+      this.lockTimeoutTime = lockTimeoutTime;
+      this.lockTimeoutTimeUnit = lockTimeoutTimeUnit;
+    }
+  }
 
   /**
    * This method is intended to be used only for testing purposes.
@@ -43,21 +62,9 @@ public class FilesystemPackageCacheManagerLocks {
    * @throws IOException
    */
   public FilesystemPackageCacheManagerLocks(File cacheFolder) throws IOException {
-    this(cacheFolder, 60L, TimeUnit.SECONDS);
-  }
-
-  private FilesystemPackageCacheManagerLocks(File cacheFolder, Long lockTimeoutTime, TimeUnit lockTimeoutTimeUnit) throws IOException {
     this.cacheFolder = cacheFolder;
-    this.lockTimeoutTime = lockTimeoutTime;
-    this.lockTimeoutTimeUnit = lockTimeoutTimeUnit;
   }
 
-  /**
-   * This method is intended to be used only for testing purposes.
-   */
-  protected FilesystemPackageCacheManagerLocks withLockTimeout(Long lockTimeoutTime, TimeUnit lockTimeoutTimeUnit) throws IOException {
-    return new FilesystemPackageCacheManagerLocks(cacheFolder, lockTimeoutTime, lockTimeoutTimeUnit);
-  }
 
   /**
    * Returns a single FilesystemPackageCacheManagerLocks instance for the given cacheFolder.
@@ -102,6 +109,19 @@ public class FilesystemPackageCacheManagerLocks {
       }
       return result;
     }
+
+    public boolean canLockFileBeHeldByThisProcess(File lockFile) throws IOException {
+      return doWriteWithLock(() -> {
+      try (FileChannel channel = new RandomAccessFile(lockFile, "rw").getChannel()) {
+        FileLock fileLock = channel.tryLock(0, Long.MAX_VALUE, false);
+        if (fileLock != null) {
+          fileLock.release();
+          channel.close();
+          return true;
+        }
+      }
+      return false;});
+    }
   }
 
   public class PackageLock {
@@ -114,15 +134,43 @@ public class FilesystemPackageCacheManagerLocks {
       this.lock = lock;
     }
 
-    private void checkForLockFileWaitForDeleteIfExists(File lockFile) throws IOException {
+    private void checkForLockFileWaitForDeleteIfExists(File lockFile, @Nonnull LockParameters lockParameters) throws IOException {
       if (!lockFile.exists()) {
         return;
       }
+
+      // Check if the file is locked by a process. If it is not, it is likely an incomplete package cache install, and
+      // we should throw an exception.
+      if (lockFile.isFile()) {
+        try (FileChannel channel = new RandomAccessFile(lockFile, "rw").getChannel()) {
+          FileLock fileLock = channel.tryLock(0, Long.MAX_VALUE, false);
+          if (fileLock != null) {
+            fileLock.release();
+            channel.close();
+            throw new IOException("Lock file exists, but is not locked by a process: " + lockFile.getName());
+          }
+          System.out.println("File is locked.");
+        }
+      }
+      try {
+        waitForLockFileDeletion(lockFile, lockParameters);
+      } catch (InterruptedException e) {
+        Thread.currentThread().interrupt();
+        throw new IOException("Thread interrupted while waiting for lock", e);
+      }
+    }
+
+    /*
+     Wait for the lock file to be deleted. If the lock file is not deleted within the timeout or if the thread is
+     interrupted, an IOException is thrown.
+     */
+    private void waitForLockFileDeletion(File lockFile, @Nonnull LockParameters lockParameters) throws IOException, InterruptedException {
+
       try (WatchService watchService = FileSystems.getDefault().newWatchService()) {
         Path dir = lockFile.getParentFile().toPath();
         dir.register(watchService, StandardWatchEventKinds.ENTRY_DELETE);
 
-        WatchKey key = watchService.poll(lockTimeoutTime, lockTimeoutTimeUnit);
+        WatchKey key = watchService.poll(lockParameters.lockTimeoutTime, lockParameters.lockTimeoutTimeUnit);
         if (key == null) {
           // It is possible that the lock file is deleted before the watch service is registered, so if we timeout at
           // this point, we should check if the lock file still exists.
@@ -141,24 +189,33 @@ public class FilesystemPackageCacheManagerLocks {
             key.reset();
           }
         }
-      } catch (InterruptedException e) {
-        Thread.currentThread().interrupt();
-        throw new IOException("Error reading package.", e);
       } catch (TimeoutException e) {
-        throw new IOException("Error reading package.", e);
+        throw new IOException("Package cache timed out waiting for lock.", e);
       }
     }
 
 
-    public <T> T doReadWithLock(FilesystemPackageCacheManager.CacheLockFunction<T> f) throws IOException {
+    /**
+     * Wraps the execution of a package read function with the appropriate cache, package, and .lock file locking and
+     * checks.
+     *
+     * @param function The function to execute
+     * @param lockParameters The parameters for the lock
+     * @return The return of the function
+     * @param <T> The return type of the function
+     * @throws IOException If an error occurs while managing locking.
+     */
+    public <T> T doReadWithLock(FilesystemPackageCacheManager.CacheLockFunction<T> function, @Nullable LockParameters lockParameters) throws IOException {
+      final LockParameters resolvedLockParameters = lockParameters != null ? lockParameters : FilesystemPackageCacheManagerLocks.lockParameters;
+
       cacheLock.getLock().readLock().lock();
       lock.readLock().lock();
 
-      checkForLockFileWaitForDeleteIfExists(lockFile);
+      checkForLockFileWaitForDeleteIfExists(lockFile, resolvedLockParameters);
 
       T result = null;
       try {
-        result = f.get();
+        result = function.get();
       } finally {
         lock.readLock().unlock();
         cacheLock.getLock().readLock().unlock();
@@ -166,35 +223,55 @@ public class FilesystemPackageCacheManagerLocks {
       return result;
     }
 
-    public <T> T doWriteWithLock(FilesystemPackageCacheManager.CacheLockFunction<T> f) throws IOException {
+    /**
+     * Wraps the execution of a package write function with the appropriate cache, package, and .lock file locking and
+     * checks.
+     *
+     * @param function The function to execute
+     * @param lockParameters The parameters for the lock
+     * @return The return of the function
+     * @param <T> The return type of the function
+     * @throws IOException If an error occurs while managing locking.
+     */
+    public <T> T doWriteWithLock(FilesystemPackageCacheManager.CacheLockFunction<T> function, @Nullable LockParameters lockParameters) throws IOException {
+      final LockParameters resolvedLockParameters = lockParameters != null ? lockParameters : FilesystemPackageCacheManagerLocks.lockParameters;
+
       cacheLock.getLock().writeLock().lock();
       lock.writeLock().lock();
 
-      if (!lockFile.isFile()) {
+       /*TODO Eventually, this logic should exist in a Lockfile class so that it isn't duplicated between the main code and
-        try {
+          the test code.
-          TextFile.stringToFile("", lockFile);
+        */
-        } catch (IOException e) {
-          e.printStackTrace();
-          return null;
-        }
-      }
       try (FileChannel channel = new RandomAccessFile(lockFile, "rw").getChannel()) {
-        FileLock fileLock = null;
+
-        while (fileLock == null) {
+        FileLock fileLock = channel.tryLock(0, Long.MAX_VALUE, false);
-          fileLock = channel.tryLock(0, Long.MAX_VALUE, true);
+
-          if (fileLock == null) {
+        if (fileLock == null) {
-            Thread.sleep(100); // Wait and retry
+          waitForLockFileDeletion(lockFile, resolvedLockParameters);
-          }
+          fileLock = channel.tryLock(0, Long.MAX_VALUE, false);
+        }
+        if (fileLock == null) {
+          throw new IOException("Failed to acquire lock on file: " + lockFile.getName());
+        }
+
+        if (!lockFile.isFile()) {
+          final ByteBuffer buff = ByteBuffer.wrap(String.valueOf(ProcessHandle.current().pid()).getBytes(StandardCharsets.UTF_8));
+          channel.write(buff);
         }
         T result = null;
         try {
-          result = f.get();
+          result = function.get();
         } finally {
+
+          lockFile.renameTo(ManagedFileAccess.file(File.createTempFile(lockFile.getName(), ".lock-renamed").getAbsolutePath()));
+
           fileLock.release();
           channel.close();
+
           if (!lockFile.delete()) {
             lockFile.deleteOnExit();
           }
+
           lock.writeLock().unlock();
           cacheLock.getLock().writeLock().unlock();
         }

org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/FilesystemPackageManagerLockTests.java

@@ -1,7 +1,7 @@
 package org.hl7.fhir.utilities.npm;
 
-import org.hl7.fhir.utilities.TextFile;
 import org.hl7.fhir.utilities.filesystem.ManagedFileAccess;
+import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
@@ -12,10 +12,12 @@ import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.Assert.assertTrue;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
 public class FilesystemPackageManagerLockTests {
@@ -49,13 +51,13 @@ public class FilesystemPackageManagerLockTests {
     packageLock.doWriteWithLock(() -> {
       assertThat(packageLock.getLockFile()).exists();
       return null;
-    });
+    }, null);
     assertThat(packageLock.getLockFile()).doesNotExist();
 
     packageLock.doReadWithLock(() -> {
       assertThat(packageLock.getLockFile()).doesNotExist();
       return null;
-    });
+    }, null);
   }
 
   @Test void testNoPackageWriteOrReadWhileWholeCacheIsLocked() throws IOException, InterruptedException {
@@ -87,7 +89,7 @@ public class FilesystemPackageManagerLockTests {
           packageLock.doWriteWithLock(() -> {
             assertThat(cacheLockFinished.get()).isTrue();
             return null;
-          });
+          }, null);
         } catch (IOException e) {
           throw new RuntimeException(e);
         }
@@ -97,7 +99,7 @@ public class FilesystemPackageManagerLockTests {
           packageLock.doReadWithLock(() -> {
             assertThat(cacheLockFinished.get()).isTrue();
             return null;
-          });
+          }, null);
         } catch (IOException e) {
           throw new RuntimeException(e);
         }
@@ -116,6 +118,23 @@ public class FilesystemPackageManagerLockTests {
     }
   }
 
+  @Test void testWhenLockIsntHeld_canLockFileBeHeldByThisProcessIsTrue() throws IOException {
+    File lockFile = getPackageLockFile();
+    lockFile.createNewFile();
+    Assertions.assertTrue(filesystemPackageCacheLockManager.getCacheLock().canLockFileBeHeldByThisProcess(lockFile));
+  }
+
+  @Test void testWhenLockIsHelp_canLockFileBeHeldByThisProcessIsFalse() throws InterruptedException, TimeoutException, IOException {
+    File lockFile = getPackageLockFile();
+    Thread lockThread = LockfileTestProcessUtility.lockWaitAndDeleteInNewProcess(cachePath, DUMMY_PACKAGE + ".lock", 2);
+
+    LockfileTestUtility.waitForLockfileCreation(cacheDirectory.getAbsolutePath(), DUMMY_PACKAGE + ".lock");
+
+    Assertions.assertFalse(filesystemPackageCacheLockManager.getCacheLock().canLockFileBeHeldByThisProcess(lockFile));
+
+    lockThread.join();
+  }
+
   @Test void testSinglePackageWriteMultiPackageRead() throws IOException {
     final FilesystemPackageCacheManagerLocks.PackageLock packageLock = filesystemPackageCacheLockManager.getPackageLock(DUMMY_PACKAGE);
     AtomicInteger writeCounter = new AtomicInteger(0);
@@ -133,7 +152,7 @@ public class FilesystemPackageManagerLockTests {
             assertThat(writeCount).isEqualTo(1);
             writeCounter.decrementAndGet();
             return null;
-          });
+          }, null);
         } catch (IOException e) {
           throw new RuntimeException(e);
         }
@@ -156,7 +175,7 @@ public class FilesystemPackageManagerLockTests {
             }
             readCounter.decrementAndGet();
             return null;
-          });
+          }, null);
         } catch (IOException e) {
           throw new RuntimeException(e);
         }
@@ -179,49 +198,47 @@ public class FilesystemPackageManagerLockTests {
   }
 
   @Test
-  public void testReadWhenLockedByFileTimesOut() throws IOException {
+  public void testReadWhenLockedByFileTimesOut() throws InterruptedException, TimeoutException, IOException {
-    FilesystemPackageCacheManagerLocks shorterTimeoutManager = filesystemPackageCacheLockManager.withLockTimeout(3L, TimeUnit.SECONDS);
+    FilesystemPackageCacheManagerLocks shorterTimeoutManager = filesystemPackageCacheLockManager;
     final FilesystemPackageCacheManagerLocks.PackageLock packageLock = shorterTimeoutManager.getPackageLock(DUMMY_PACKAGE);
-    File lockFile = createPackageLockFile();
+    File lockFile = getPackageLockFile();
+    Thread lockThread = LockfileTestProcessUtility.lockWaitAndDeleteInNewProcess(cachePath, lockFile.getName(), 5);
+    LockfileTestUtility.waitForLockfileCreation(cachePath,lockFile.getName());
 
     Exception exception = assertThrows(IOException.class, () -> {
       packageLock.doReadWithLock(() -> {
         assertThat(lockFile).exists();
         return null;
-      });
+      }, new FilesystemPackageCacheManagerLocks.LockParameters(3L, TimeUnit.SECONDS));
     });
 
-    assertThat(exception.getMessage()).contains("Error reading package");
+    assertThat(exception.getMessage()).contains("Package cache timed out waiting for lock");
     assertThat(exception.getCause().getMessage()).contains("Timeout waiting for lock file deletion: " + lockFile.getName());
+
+    lockThread.join();
   }
 
-  @Test
-  public void testReadWhenLockFileIsDeleted() throws IOException {
-    FilesystemPackageCacheManagerLocks shorterTimeoutManager = filesystemPackageCacheLockManager.withLockTimeout(5L, TimeUnit.SECONDS);
-    final FilesystemPackageCacheManagerLocks.PackageLock packageLock = shorterTimeoutManager.getPackageLock(DUMMY_PACKAGE);
-    File lockFile = createPackageLockFile();
 
-    Thread t = new Thread(() -> {
+  @Test
-      try {
+  public void testReadWhenLockFileIsDeleted() throws InterruptedException, TimeoutException, IOException {
-        Thread.sleep(2000);
+
-      } catch (InterruptedException e) {
+    final FilesystemPackageCacheManagerLocks.PackageLock packageLock = filesystemPackageCacheLockManager.getPackageLock(DUMMY_PACKAGE);
-        throw new RuntimeException(e);
+
-      }
+    final File lockFile = getPackageLockFile();
-      lockFile.delete();
+
-    });
+    Thread lockThread = LockfileTestProcessUtility.lockWaitAndDeleteInNewProcess(cachePath, lockFile.getName(), 5);
-    t.start();
+    LockfileTestUtility.waitForLockfileCreation(cachePath,lockFile.getName());
 
     packageLock.doReadWithLock(() -> {
       assertThat(lockFile).doesNotExist();
       return null;
-    });
+    }, new FilesystemPackageCacheManagerLocks.LockParameters(10L, TimeUnit.SECONDS));
 
+    lockThread.join();
   }
 
-  private File createPackageLockFile() throws IOException {
+  private File getPackageLockFile() {
-    File lockFile = Path.of(cachePath, DUMMY_PACKAGE + ".lock").toFile();
+    return Path.of(cachePath, DUMMY_PACKAGE + ".lock").toFile();
-    TextFile.stringToFile("", lockFile);
-    return lockFile;
   }
 
 }

org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/FilesystemPackageManagerTests.java

@@ -2,6 +2,7 @@ package org.hl7.fhir.utilities.npm;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import java.io.File;
 
@@ -13,6 +14,8 @@ import java.util.List;
 import java.util.Random;
 import java.util.UUID;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.stream.Stream;
 
@@ -20,6 +23,7 @@ import javax.annotation.Nonnull;
 
 import org.hl7.fhir.utilities.IniFile;
 import org.hl7.fhir.utilities.filesystem.ManagedFileAccess;
+import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.condition.DisabledOnOs;
 import org.junit.jupiter.api.condition.EnabledOnOs;
@@ -113,6 +117,97 @@ public class FilesystemPackageManagerTests {
     assertEquals( System.getenv("ProgramData") + "\\.fhir\\packages", folder.getAbsolutePath());
   }
 
+  @Test
+  public void testCorruptPackageCleanup() throws IOException {
+    File cacheDirectory = ManagedFileAccess.fromPath(Files.createTempDirectory("fpcm-multithreadingTest"));
+
+    File dummyPackage = createDummyPackage(cacheDirectory, "example.fhir.uv.myig", "1.2.3");
+    File dummyLockFile = createDummyLockFile(cacheDirectory, "example.fhir.uv.myig" , "1.2.3");
+
+    assertThat(dummyPackage).isDirectory();
+    assertThat(dummyPackage).exists();
+    assertThat(dummyLockFile).exists();
+
+    FilesystemPackageCacheManager filesystemPackageCacheManager = new FilesystemPackageCacheManager.Builder().withCacheFolder(cacheDirectory.getAbsolutePath()).build();
+
+    assertThat(dummyPackage).doesNotExist();
+    assertThat(dummyLockFile).doesNotExist();
+  }
+
+  @Test
+  public void testLockedPackageIsntCleanedUp() throws IOException, InterruptedException, TimeoutException {
+    File cacheDirectory = ManagedFileAccess.fromPath(Files.createTempDirectory("fpcm-multithreadingTest"));
+
+    File dummyPackage = createDummyPackage(cacheDirectory, "example.fhir.uv.myig", "1.2.3");
+
+    Thread lockThread = LockfileTestProcessUtility.lockWaitAndDeleteInNewProcess(cacheDirectory.getAbsolutePath(), "example.fhir.uv.myig#1.2.3.lock", 2);
+
+    LockfileTestUtility.waitForLockfileCreation(cacheDirectory.getAbsolutePath(), "example.fhir.uv.myig#1.2.3.lock");
+    File dummyLockFile = ManagedFileAccess.file(cacheDirectory.getAbsolutePath(), "example.fhir.uv.myig#1.2.3.lock");
+
+    assertThat(dummyPackage).isDirectory();
+    assertThat(dummyPackage).exists();
+    assertThat(dummyLockFile).exists();
+
+    FilesystemPackageCacheManager filesystemPackageCacheManager = new FilesystemPackageCacheManager.Builder().withCacheFolder(cacheDirectory.getAbsolutePath()).build();
+
+    assertThat(dummyPackage).exists();
+    assertThat(dummyLockFile).exists();
+
+    lockThread.join();
+  }
+
+  @Test
+  public void testTimeoutForLockedPackageRead() throws IOException, InterruptedException, TimeoutException {
+    String pcmPath = ManagedFileAccess.fromPath(Files.createTempDirectory("fpcm-multithreadingTest")).getAbsolutePath();
+
+    final FilesystemPackageCacheManager pcm = new FilesystemPackageCacheManager.Builder()
+      .withCacheFolder(pcmPath)
+      .withLockParameters(new FilesystemPackageCacheManagerLocks.LockParameters(5,TimeUnit.SECONDS))
+      .build();
+
+    Assertions.assertTrue(pcm.listPackages().isEmpty());
+
+    Thread lockThread = LockfileTestProcessUtility.lockWaitAndDeleteInNewProcess(pcmPath, "example.fhir.uv.myig#1.2.3.lock", 10);
+    File directory = ManagedFileAccess.file(pcmPath, "example.fhir.uv.myig#1.2.3" );
+    directory.mkdir();
+
+    LockfileTestUtility.waitForLockfileCreation(pcmPath, "example.fhir.uv.myig#1.2.3.lock");
+
+    IOException exception = assertThrows(IOException.class, () -> pcm.loadPackageFromCacheOnly("example.fhir.uv.myig", "1.2.3"));
+
+    assertThat(exception.getMessage()).contains("Package cache timed out waiting for lock");
+    assertThat(exception.getCause().getMessage()).contains("Timeout waiting for lock file deletion");
+    lockThread.join();
+  }
+
+  @Test
+  public void testReadFromCacheOnlyWaitsForLockDelete() throws IOException, InterruptedException, TimeoutException {
+    String pcmPath = ManagedFileAccess.fromPath(Files.createTempDirectory("fpcm-multithreadingTest")).getAbsolutePath();
+
+    final FilesystemPackageCacheManager pcm = new FilesystemPackageCacheManager.Builder().withCacheFolder(pcmPath).build();
+
+    Assertions.assertTrue(pcm.listPackages().isEmpty());
+
+    pcm.addPackageToCache("example.fhir.uv.myig", "1.2.3", this.getClass().getResourceAsStream("/npm/dummy-package.tgz"), "https://packages.fhir.org/example.fhir.uv.myig/1.2.3");
+
+    String packageAndVersion = "example.fhir.uv.myig#1.2.3";
+
+    //Now sneak in a new lock file and directory:
+
+    File directory = ManagedFileAccess.file(pcmPath, packageAndVersion);
+    directory.mkdir();
+
+    Thread lockThread = LockfileTestProcessUtility.lockWaitAndDeleteInNewProcess(pcmPath, "example.fhir.uv.myig#1.2.3.lock", 5);
+    LockfileTestUtility.waitForLockfileCreation(pcmPath, "example.fhir.uv.myig#1.2.3.lock");
+
+    NpmPackage npmPackage = pcm.loadPackageFromCacheOnly("example.fhir.uv.myig", "1.2.3");
+
+    assertThat(npmPackage.id()).isEqualTo("example.fhir.uv.myig");
+
+    lockThread.join();
+  }
+
   /**
     We repeat the same tests multiple times here, in order to catch very rare edge cases.
    */
@@ -126,16 +221,16 @@ public class FilesystemPackageManagerTests {
     return params.stream();
   }
 
-  private void createDummyTemp(File cacheDirectory, String lowerCase) throws IOException {
+  private File createDummyTemp(File cacheDirectory, String lowerCase) throws IOException {
-    createDummyPackage(cacheDirectory, lowerCase);
+    return createDummyPackage(cacheDirectory, lowerCase);
   }
 
-  private void createDummyPackage(File cacheDirectory, String packageName, String packageVersion) throws IOException {
+  private File createDummyPackage(File cacheDirectory, String packageName, String packageVersion) throws IOException {
     String directoryName = packageName + "#" + packageVersion;
-    createDummyPackage(cacheDirectory, directoryName);
+    return createDummyPackage(cacheDirectory, directoryName);
   }
 
-  private static void createDummyPackage(File cacheDirectory, String directoryName) throws IOException {
+  private static File createDummyPackage(File cacheDirectory, String directoryName) throws IOException {
     File packageDirectory = ManagedFileAccess.file(cacheDirectory.getAbsolutePath(), directoryName);
     packageDirectory.mkdirs();
 
@@ -144,6 +239,16 @@ public class FilesystemPackageManagerTests {
     wr.write("Ain't nobody here but us chickens");
     wr.flush();
     wr.close();
+    return packageDirectory;
+  }
+
+  private File createDummyLockFile(File cacheDirectory, String packageName, String packageVersion) throws IOException {
+    final File dummyLockFile = ManagedFileAccess.file(cacheDirectory.getAbsolutePath(), packageName + "#" + packageVersion + ".lock");
+    final FileWriter wr = new FileWriter(dummyLockFile);
+    wr.write("Ain't nobody here but us chickens");
+    wr.flush();
+    wr.close();
+    return dummyLockFile;
   }
 
   private void assertThatDummyTempExists(File cacheDirectory, String dummyTempPackage) throws IOException {
@@ -241,13 +346,13 @@ public class FilesystemPackageManagerTests {
   @MethodSource("packageCacheMultiThreadTestParams")
   @ParameterizedTest
   public void packageCacheMultiThreadTest(final int threadTotal, final int packageCacheManagerTotal) throws IOException {
-
     String pcmPath = ManagedFileAccess.fromPath(Files.createTempDirectory("fpcm-multithreadingTest")).getAbsolutePath();
+    System.out.println("Using temp pcm path: " + pcmPath);
     FilesystemPackageCacheManager[] packageCacheManagers = new FilesystemPackageCacheManager[packageCacheManagerTotal];
     Random rand = new Random();
 
     final AtomicInteger totalSuccessful = new AtomicInteger();
-    final ConcurrentHashMap successfulThreads = new ConcurrentHashMap();
+    final ConcurrentHashMap<Long, Integer> successfulThreads = new ConcurrentHashMap<>();
     List<Thread> threads = new ArrayList<>();
     for (int i = 0; i < threadTotal; i++) {
       final int index = i;
@@ -256,22 +361,27 @@ public class FilesystemPackageManagerTests {
           System.out.println("Thread #" + index + ": " + Thread.currentThread().getId() + " started");
           final int randomPCM = rand.nextInt(packageCacheManagerTotal);
           final int randomOperation = rand.nextInt(4);
+          final String operationName;
           if (packageCacheManagers[randomPCM] == null) {
             packageCacheManagers[randomPCM] = new FilesystemPackageCacheManager.Builder().withCacheFolder(pcmPath).build();
           }
           FilesystemPackageCacheManager pcm = packageCacheManagers[randomPCM];
           if (randomOperation == 0) {
+            operationName = "addPackageToCache";
             pcm.addPackageToCache("example.fhir.uv.myig", "1.2.3", this.getClass().getResourceAsStream("/npm/dummy-package.tgz"), "https://packages.fhir.org/example.fhir.uv.myig/1.2.3");
           } else if (randomOperation == 1) {
+            operationName = "clear";
             pcm.clear();
           } else if (randomOperation == 2) {
+            operationName = "loadPackageFromCacheOnly";
             pcm.loadPackageFromCacheOnly("example.fhir.uv.myig", "1.2.3");
           } else {
+            operationName = "removePackage";
             pcm.removePackage("example.fhir.uv.myig", "1.2.3");
           }
           totalSuccessful.incrementAndGet();
           successfulThreads.put(Thread.currentThread().getId(), index);
-          System.out.println("Thread #" + index + ": " + Thread.currentThread().getId() + " completed");
+          System.out.println("Thread #" + index + ": " + Thread.currentThread().getId() + " completed. Ran: " + operationName);
         } catch (Exception e) {
           e.printStackTrace();
           System.err.println("Thread #" + index + ": " + Thread.currentThread().getId() + " failed");

org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestProcessUtility.java

@@ -0,0 +1,122 @@
+package org.hl7.fhir.utilities.npm;
+
+import org.hl7.fhir.utilities.filesystem.ManagedFileAccess;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.channels.FileLock;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Paths;
+
+/**
+ * FilesystemPackageCacheManagerLocks relies on the existence of .lock files to prevent access to packages being written
+ * by processes outside the current JVM. Testing this functionality means creating a process outside the JUnit test JVM,
+ * which is achieved by running a separate Java process.
+ * <p/>
+ * Intended usage:
+ * <p/>
+ * The helper method {@link #lockWaitAndDeleteInNewProcess(String, String, int)} is the intended starting point for
+ * using this class.
+ * <p/>
+ *
+ *
+ * This class deliberately avoids using any dependencies outside java.*, which avoids having to construct a classpath
+ * for the separate process.
+ */
+public class LockfileTestProcessUtility {
+  /**
+   * Main method to allow running this class.
+   * <p/
+   * It is not recommended to call this method directly. Instead, use the provided {@link #lockWaitAndDeleteInNewProcess(String, String, int)} method.
+   *
+   * @param args The arguments to the main method. The first argument is the path to create the lockfile in, the second
+   *             argument is the name of the lockfile, and the third argument is the number of seconds to wait before
+   *             deleting the lockfile.
+   */
+  public static void main(String[] args) {
+
+    String path = args[0];
+    String lockFileName = args[1];
+    int seconds = Integer.parseInt(args[2]);
+
+    try {
+      lockWaitAndDelete(path, lockFileName, seconds);
+    } catch (InterruptedException | IOException e) {
+      throw new RuntimeException(e);
+    }
+
+  }
+
+  /**
+   * Static helper method that starts a new process, creates a lock file in the path and waits for a specified number of
+   * seconds before deleting it.
+   * <p/>
+   * This method calls the {@link #main(String[])} method in a new process.
+   *
+   * @param path The path to create the lockfile in
+   * @param lockFileName The name of the lockfile
+   * @param seconds The number of seconds to wait before deleting the lockfile
+   * @return The thread wrapping the process execution. This can be used to wait for the process to complete, so that
+   * System.out and System.err can be processed before tests return results.
+   */
+  public static Thread lockWaitAndDeleteInNewProcess(String path, String lockFileName, int seconds)  {
+    Thread t = new Thread(() -> {
+      ProcessBuilder processBuilder = new ProcessBuilder("java", "-cp", "target/test-classes", LockfileTestProcessUtility.class.getName(), path, lockFileName, Integer.toString(seconds));
+      try {
+        Process process = processBuilder.start();
+        process.getErrorStream().transferTo(System.err);
+        process.getInputStream().transferTo(System.out);
+        process.waitFor();
+      } catch (IOException | InterruptedException e) {
+        throw new RuntimeException(e);
+      }
+    });
+    t.start();
+    return t;
+  }
+
+  /**
+   * The actual logic to create a .lock file.
+   * <p/>
+   * This should match the logic in FilesystemPackageCacheManagerLocks
+   * <p/>
+   *
+   * @param path The path to create the lockfile in
+   * @param lockFileName The name of the lockfile
+   * @param seconds The number of seconds to wait before deleting the lockfile
+   * @throws InterruptedException If the thread is interrupted while waiting
+   * @throws IOException If there is an error accessing the file system
+   */
+  /* TODO Eventually, this logic should exist in a Lockfile class so that it isn't duplicated between the main code and
+      the test code.
+   */
+  private static void lockWaitAndDelete(String path, String lockFileName, int seconds) throws InterruptedException, IOException {
+
+    File lockFile = Paths.get(path,lockFileName).toFile();
+
+    try (FileChannel channel = new RandomAccessFile(lockFile.getAbsolutePath(), "rw").getChannel()) {
+      FileLock fileLock = channel.tryLock(0, Long.MAX_VALUE, false);
+      if (fileLock != null) {
+        final ByteBuffer buff = ByteBuffer.wrap("Hello world".getBytes(StandardCharsets.UTF_8));
+        channel.write(buff);
+        System.out.println("File "+lockFileName+" is locked. Waiting for " + seconds + " seconds to release. ");
+        Thread.sleep(seconds * 1000L);
+
+        lockFile.renameTo(ManagedFileAccess.file(File.createTempFile(lockFile.getName(), ".lock-renamed").getAbsolutePath()));
+
+        fileLock.release();
+        channel.close();
+        System.out.println(System.currentTimeMillis());
+        System.out.println("File "+lockFileName+" is released.");
+
+        lockFile.delete();
+      }}finally {
+      if (lockFile.exists()) {
+        lockFile.delete();
+      }
+    }
+  }
+}

org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java

@@ -0,0 +1,78 @@
+package org.hl7.fhir.utilities.npm;
+
+
+import org.apache.commons.io.monitor.FileAlterationListenerAdaptor;
+import org.apache.commons.io.monitor.FileAlterationMonitor;
+import org.apache.commons.io.monitor.FileAlterationObserver;
+
+import java.io.*;
+
+import java.nio.file.*;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+
+public class LockfileTestUtility {
+
+
+
+  /**
+   * Wait for the lock file to be created in the given path.
+   * <p/>
+   * Normally, within the same JVM, you could use a CountdownLatch for the same purpose, but since this the lock file is
+   * being created in a separate process, we need to use a mechanism that doesn't rely on shared threads.
+   *
+   * @param path The path containing the lock file
+   * @param lockFileName The name of the lock file
+   * @throws InterruptedException If the thread is interrupted while waiting
+   * @throws TimeoutException If the lock file is not created within 10 seconds
+   */
+  public static void waitForLockfileCreation(String path, String lockFileName) throws InterruptedException, TimeoutException {
+
+    CountDownLatch latch = new CountDownLatch(1);
+    FileAlterationMonitor monitor = new FileAlterationMonitor(100);
+    FileAlterationObserver observer = new FileAlterationObserver(path);
+
+    observer.addListener(new FileAlterationListenerAdaptor(){
+
+      @Override
+      public void onStart(FileAlterationObserver observer) {
+        if (Files.exists(Paths.get(path, lockFileName))) {
+          latch.countDown();
+        }
+      }
+
+      @Override
+      public void onFileCreate(File file) {
+        System.out.println("File created: " + file.getName());
+        latch.countDown();
+      }
+    });
+    monitor.addObserver(observer);
+
+      try {
+        monitor.start();
+      } catch (Exception e) {
+        throw new RuntimeException(e);
+      }
+      boolean success = latch.await(10, TimeUnit.SECONDS);
+      try {
+        monitor.stop();
+      } catch (Exception e) {
+        throw new RuntimeException(e);
+      }
+
+      if (!success) {
+        throw new TimeoutException("Timed out waiting for lock file creation: " + lockFileName);
+      }
+      // TODO This is a workaround for an edge condition that shows up with testing, where the lock is not reflected in
+      //  the file system immediately. It is unlikely to appear in production environments. Should it occur, it will
+      //  result in a lock file being erroneously reported as not having an owning process, and will cause a package to
+      //  fail to be loaded from that cache until the lock is cleaned up by cache initialization.
+      Thread.sleep(100);
+
+  }
+
+
+}