From 1c4abbbcbd047fcdf2d396a129900b1a49194618 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mai=20Huy=20Ho=C3=A0ng?= Date: Sun, 9 Aug 2026 11:39:10 +0700 Subject: [PATCH] [2.x] fix: Don't re-extract a dirzip whose digest already matches (#9555) syncFile treated an output as up to date only when its digest matched and the path was already a symlink. packageDirectory installs the archive with Files.move, so on the write path it is always a regular file and the second conjunct never held. Every compile therefore fell through to IO.delete + writeFileAndNotify -> afterFileWrite -> unpackageDirZip, which inflated the archive it had just written into a temp directory, digest-compared every file in the output tree, found all of them unchanged, and deleted the temp directory. The cost is proportional to output size and is paid on every compile of every module. Measured at ~450 ms for a 2557-file, 33 MB classes directory (11 MB archive), against a 4.2 s warm single-file edit. A matching digest already means the local content is the blob's, whether or not the path is a symlink, so take the up-to-date path in that case. Where the path is not yet a symlink, still relink it to the CAS to keep deduplication, but reach afterFileUpToDate rather than afterFileWrite. The symlink-creation fallback is factored into linkOrCopy and shared with writeFileAndNotify. For non-dirzip outputs both hooks are no-ops, and that branch already did a delete-and-relink, so their behaviour is unchanged. The behaviour given up is that a file inside the extracted directory which diverged from the manifest is no longer silently restored while the archive digest still matches; on a cache hit with a differing digest the full unpackage runs as before. Co-Authored-By: Claude Opus 5 (1M context) --- .../scala/sbt/util/ActionCacheStore.scala | 18 +++--- .../test/scala/sbt/util/ActionCacheTest.scala | 57 +++++++++++++++++++ 2 files changed, 67 insertions(+), 8 deletions(-) diff --git a/util-cache/src/main/scala/sbt/util/ActionCacheStore.scala b/util-cache/src/main/scala/sbt/util/ActionCacheStore.scala index 04f02a12a..77cd12a91 100644 --- a/util-cache/src/main/scala/sbt/util/ActionCacheStore.scala +++ b/util-cache/src/main/scala/sbt/util/ActionCacheStore.scala @@ -318,9 +318,8 @@ case class DiskActionCacheStore(base: Path, converter: FileConverter) // See https://github.com/sbt/sbt/issues/7656 // On Windows, the program has be running under the Administrator privileges or the // user enable Developer Mode on Windows 10+ to create symbolic links. - def writeFileAndNotify(outPath: Path): Path = - Option(outPath.getParent()).foreach(parent => IO.createDirectory(parent.toFile())) - val result = Retry: + def linkOrCopy(outPath: Path): Path = + Retry: if Files.exists(outPath) then IO.delete(outPath.toFile()) if symlinkSupported.get() && Files.exists(casFile) then try Files.createSymbolicLink(outPath, casFile) @@ -339,6 +338,9 @@ case class DiskActionCacheStore(base: Path, converter: FileConverter) symlinkSupported.set(false) copyFile(outPath) else copyFile(outPath) + def writeFileAndNotify(outPath: Path): Path = + Option(outPath.getParent()).foreach(parent => IO.createDirectory(parent.toFile())) + val result = linkOrCopy(outPath) afterFileWrite(ref, result, outputDirectory) result val resolvedPath = converter.toPath(ref) match @@ -350,11 +352,11 @@ case class DiskActionCacheStore(base: Path, converter: FileConverter) writeFileAndNotify(p) case p => try - // `!symlinkSupported` prevents unnecessary deletion of files and then copying them again - // in #writeFileAndNotify on machines that don't support symlinks. - if Digest.sameDigest(p, d) && (!symlinkSupported.get() || Files.isSymbolicLink(p)) then - afterFileUpToDate(ref, p, outputDirectory) - p + if Digest.sameDigest(p, d) then + val result = + if symlinkSupported.get() && !Files.isSymbolicLink(p) then linkOrCopy(p) else p + afterFileUpToDate(ref, result, outputDirectory) + result else // println(s"- syncFile: $p has different digest") IO.delete(p.toFile()) diff --git a/util-cache/src/test/scala/sbt/util/ActionCacheTest.scala b/util-cache/src/test/scala/sbt/util/ActionCacheTest.scala index 6018036d7..82adab94c 100644 --- a/util-cache/src/test/scala/sbt/util/ActionCacheTest.scala +++ b/util-cache/src/test/scala/sbt/util/ActionCacheTest.scala @@ -115,6 +115,63 @@ object ActionCacheTest extends BasicTestSuite: assert((dir / "a.txt").exists, "a.txt not re-extracted after the directory was deleted") assert((dir / "b.txt").exists, "b.txt not re-extracted after the directory was deleted") + test("Disk cache does not re-extract a dirzip whose archive digest already matches"): + withDiskCache: cache => + IO.withTemporaryDirectory: tempDir => + val outputDirectory = tempDir.toPath() + val dir = tempDir / "gen-dir" + IO.write(dir / "a.txt", "contents A") + val zipVf = ActionCache.packageDirectory( + binaryFileConverter.toVirtualFile(dir.toPath()), + binaryFileConverter, + outputDirectory, + ) + val refs = cache.putBlobs(Seq(zipVf)) + + // packageDirectory leaves a regular file whose digest already matches the blob, so the + // extracted tree is in sync by construction and syncing must not unpackage it again. + IO.write(dir / "a.txt", "diverged") + cache.syncBlobs(refs, outputDirectory) + assert(IO.read(dir / "a.txt") == "diverged") + + test("Disk cache relinks a digest-matching dirzip to the CAS"): + withDiskCache: cache => + IO.withTemporaryDirectory: tempDir => + val outputDirectory = tempDir.toPath() + val dir = tempDir / "gen-dir" + IO.write(dir / "a.txt", "contents A") + val zipVf = ActionCache.packageDirectory( + binaryFileConverter.toVirtualFile(dir.toPath()), + binaryFileConverter, + outputDirectory, + ) + val refs = cache.putBlobs(Seq(zipVf)) + val zipPath = Paths.get(dir.toString + ActionCache.dirZipExt) + assert(!Files.isSymbolicLink(zipPath), "packageDirectory should leave a regular file") + + cache.syncBlobs(refs, outputDirectory) + assert(Files.isSymbolicLink(zipPath), "digest-matching archive was not relinked to the CAS") + + test("Disk cache re-extracts a dirzip whose archive digest differs"): + withDiskCache: cache => + IO.withTemporaryDirectory: tempDir => + val outputDirectory = tempDir.toPath() + val dir = tempDir / "gen-dir" + IO.write(dir / "a.txt", "contents A") + val zipVf = ActionCache.packageDirectory( + binaryFileConverter.toVirtualFile(dir.toPath()), + binaryFileConverter, + outputDirectory, + ) + val refs = cache.putBlobs(Seq(zipVf)) + + IO.write(Paths.get(dir.toString + ActionCache.dirZipExt).toFile(), "not an archive") + IO.write(dir / "a.txt", "diverged") + IO.write(dir / "stray.txt", "stray") + cache.syncBlobs(refs, outputDirectory) + assert(IO.read(dir / "a.txt") == "contents A", "diverged file was not restored") + assert(!(dir / "stray.txt").exists, "stray file was not removed") + test("In-memory cache can hold action value"): withInMemoryCache(testActionCacheBasic)