From 3e7ced69e0ba1f438126db539a72b1a6dfa9ff1e Mon Sep 17 00:00:00 2001
From: Ankit Aggarwal <ankit_aggarwal@apple.com>
Date: Tue, 25 Aug 2020 17:07:11 -0700
Subject: [PATCH] [PackageLoading] Add a method to properly reset manifest
 cache (#2888)

Workspace's reset method removes the cache directory while the sqlite
database is in use, which is a considered a bug in client of sqlite
databases. This exposes a proper way to reset a cache that might be used
by a particular manifest loader implementation.

<rdar://problem/67712262>
---
 Sources/PackageLoading/ManifestLoader.swift   | 33 +++++++++++++++----
 Sources/Workspace/Workspace.swift             |  1 +
 .../PD4_2LoadingTests.swift                   |  5 +++
 3 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/Sources/PackageLoading/ManifestLoader.swift b/Sources/PackageLoading/ManifestLoader.swift
index 1870713f2f4..df86c18d3c0 100644
--- a/Sources/PackageLoading/ManifestLoader.swift
+++ b/Sources/PackageLoading/ManifestLoader.swift
@@ -77,6 +77,9 @@ public protocol ManifestLoaderProtocol {
         fileSystem: FileSystem?,
         diagnostics: DiagnosticsEngine?
     ) throws -> Manifest
+
+    /// Reset any internal cache held by the manifest loader.
+    func resetCache() throws
 }
 
 extension ManifestLoaderProtocol {
@@ -112,6 +115,9 @@ extension ManifestLoaderProtocol {
             diagnostics: diagnostics
         )
     }
+
+    public func resetCache() throws {
+    }
 }
 
 public protocol ManifestLoaderDelegate {
@@ -136,7 +142,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
     }
     let cacheDir: AbsolutePath!
     let delegate: ManifestLoaderDelegate?
-    let cache: PersistentCacheProtocol?
+    private(set) var cache: PersistentCacheProtocol?
 
     public init(
         manifestResources: ManifestResourceProvider,
@@ -155,11 +161,6 @@ public final class ManifestLoader: ManifestLoaderProtocol {
             try? localFileSystem.createDirectory(cacheDir, recursive: true)
         }
         self.cacheDir = cacheDir.map(resolveSymlinks)
-
-        self.cache = cacheDir.flatMap {
-            // FIXME: It would be nice to emit a warning if we weren't able to create the cache.
-            try? SQLiteBackedPersistentCache(cacheFilePath: $0.appending(component: "manifest.db"))
-        }
     }
 
     @available(*, deprecated)
@@ -237,6 +238,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
         fileSystem: FileSystem? = nil,
         diagnostics: DiagnosticsEngine? = nil
     ) throws -> Manifest {
+        try self.createCacheIfNeeded()
 
         // Inform the delegate.
         self.delegate?.willLoad(manifest: inputPath)
@@ -836,6 +838,25 @@ public final class ManifestLoader: ManifestLoaderProtocol {
         // Bin dir will be set when developing swiftpm without building all of the runtimes.
         return resources.binDir ?? resources.libDir.appending(version.runtimeSubpath)
     }
+
+    /// Returns path to the manifest database inside the given cache directory.
+    private static func manifestCacheDBPath(_ cacheDir: AbsolutePath) -> AbsolutePath {
+        return cacheDir.appending(component: "manifest.db")
+    }
+
+    func createCacheIfNeeded() throws {
+        // Return if we have already created the cache.
+        guard self.cache == nil else { return }
+        guard let manifestCacheDBPath = cacheDir.flatMap({ Self.manifestCacheDBPath($0) }) else { return }
+        self.cache = try SQLiteBackedPersistentCache(cacheFilePath: manifestCacheDBPath)
+    }
+
+    public func resetCache() throws {
+        guard let manifestCacheDBPath = cacheDir.flatMap({ Self.manifestCacheDBPath($0) }) else { return }
+        self.cache = nil
+        // Also remove the database file from disk.
+        try localFileSystem.removeFileTree(manifestCacheDBPath)
+    }
 }
 
 /// Returns the sandbox profile to be used when parsing manifest on macOS.
diff --git a/Sources/Workspace/Workspace.swift b/Sources/Workspace/Workspace.swift
index b41079b9a71..0769e35b8d1 100644
--- a/Sources/Workspace/Workspace.swift
+++ b/Sources/Workspace/Workspace.swift
@@ -625,6 +625,7 @@ extension Workspace {
         guard removed else { return }
 
         repositoryManager.reset()
+        try? manifestLoader.resetCache()
         try? fileSystem.removeFileTree(dataPath)
     }
 
diff --git a/Tests/PackageLoadingTests/PD4_2LoadingTests.swift b/Tests/PackageLoadingTests/PD4_2LoadingTests.swift
index 739d0ebbd09..0d17bac0862 100644
--- a/Tests/PackageLoadingTests/PD4_2LoadingTests.swift
+++ b/Tests/PackageLoadingTests/PD4_2LoadingTests.swift
@@ -570,6 +570,11 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
             for _ in 0..<2 {
                 check(loader: noCacheLoader, expectCached: false)
             }
+
+            // Resetting the cache should allow us to remove the cache
+            // directory without triggering assertions in sqlite.
+            try manifestLoader.resetCache()
+            try localFileSystem.removeFileTree(path)
         }
     }