From 98bfcf44ced52fd1719d3d25a837984e7d5d6d79 Mon Sep 17 00:00:00 2001
From: Justin Chadwell <me@jedevc.com>
Date: Mon, 5 Feb 2024 13:41:26 +0000
Subject: [PATCH] exec: refactor content-based cache detection

This refactors the content-based cache to be that little bit tidier. In
addition to adding comments that explain *why* we're even bothering,
this restructures the code to avoid being unclear.

To explain the changes in a little more detail (since it's not
abundantly clear why this translation is valid), the initial condition
looks like:

	if (!m.Readonly || m.Dest == pb.RootMount) && m.Output != -1 {
		deps[m.Input].NoContentBasedHash = true

We can apply De Morgan's law recursively to invert the condition and the
result:

	deps[m.Input].NoContentBasedHash = true
	if (m.Readonly && m.Dest != pb.RootMount) || m.Output == -1 {
		deps[m.Input].NoContentBasedHash = false

With all the juggling of NoContentBasedCache, we invert the variable
name to be ContentBasedCache (and invert everywhere it's used as well):

	if (m.Readonly && m.Dest != pb.RootMount) || m.Output == -1 {
		deps[m.Input].ContentBasedHash = true

This reads a bit easier, but now we split this into two separate
branches for readability (and so we can comment each one in more detail
separately):

	if m.Readonly && m.Dest != pb.RootMount {
		deps[m.Input].ContentBasedHash = true
	}
	if m.Output == -1 {
		deps[m.Input].ContentBasedHash = true
	}

While this has been the behavior for ages, I think it makes sense to
deliberately this behavior slightly. It doesn't make sense to me that we
should only disallow read-only root mounts, but no-output root mounts
are allowed - there's no reason these shouldn't behave identically, by
splitting these out:

	if m.Readonly {
		deps[m.Input].ContentBasedHash = true
	}
	if m.Output == -1 {
		deps[m.Input].ContentBasedHash = true
	}

	if m.Dest == pb.RootMount {
		deps[m.Input].ContentBasedHash = false
	}

There is a small chance that this is a breaking change for some users,
however, 1. SkipOutput (-1) is very rare and not often used in the wild
(except for dockerfiles, where it's only used for non-root mounts), and 2.
will only cause a cache miss.

Signed-off-by: Justin Chadwell <me@jedevc.com>
---
 solver/llbsolver/ops/exec.go | 42 +++++++++++++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 5 deletions(-)

diff --git a/solver/llbsolver/ops/exec.go b/solver/llbsolver/ops/exec.go
index e7bc293a8..6becead59 100644
--- a/solver/llbsolver/ops/exec.go
+++ b/solver/llbsolver/ops/exec.go
@@ -212,7 +212,7 @@ func (e *ExecOp) CacheMap(ctx context.Context, g session.Group, index int) (*sol
 			}
 			cm.Deps[i].Selector = digest.FromBytes(bytes.Join(dgsts, []byte{0}))
 		}
-		if !dep.NoContentBasedHash {
+		if dep.ContentBasedHash {
 			cm.Deps[i].ComputeDigestFunc = opsutils.NewContentHashFunc(toSelectors(dedupePaths(dep.Selectors)))
 		}
 		cm.Deps[i].PreprocessFunc = unlazyResultFunc
@@ -275,8 +275,11 @@ func toSelectors(p []string) []opsutils.Selector {
 }
 
 type dep struct {
-	Selectors          []string
-	NoContentBasedHash bool
+	Selectors []string
+
+	// ContentBasedHash enables content-based caching. This is used to ensure
+	// that all caching is done safely and efficiently.
+	ContentBasedHash bool
 }
 
 func (e *ExecOp) getMountDeps() ([]dep, error) {
@@ -292,9 +295,38 @@ func (e *ExecOp) getMountDeps() ([]dep, error) {
 		sel := path.Join("/", m.Selector)
 		deps[m.Input].Selectors = append(deps[m.Input].Selectors, sel)
 
-		if (!m.Readonly || m.Dest == pb.RootMount) && m.Output != -1 { // exclude read-only rootfs && read-write mounts
-			deps[m.Input].NoContentBasedHash = true
+		// Assume that we *cannot* perform content-based caching, and then
+		// enable it selectively only for cases where we want to
+		contentBasedCache := false
+
+		// Allow content-based cached where safe - these are enforced to avoid
+		// the following case:
+		// - A "snapshot" contains "foo/a.txt" and "bar/b.txt"
+		// - "RUN --mount from=snapshot,src=bar touch bar/c.txt" creates a new
+		//   file in bar
+		// - If we run again, but this time "snapshot" contains a new
+		//   "foo/sneaky.txt", the content-based cache matches the previous
+		//   run, since we only select "bar"
+		// - But this cached result is incorrect - "foo/sneaky.txt" isn't in
+		//   our cached result, but it is in our input.
+		if m.Output == pb.SkipOutput {
+			// if the mount has no outputs, it's safe to enable content-based
+			// caching, since it's guaranteed to not be used as an input for
+			// any future steps
+			contentBasedCache = true
+		} else if m.Readonly {
+			// if the mount is read-only, then it's also safe, since it can't
+			// be modified by the operation
+			contentBasedCache = true
 		}
+
+		if m.Dest == pb.RootMount {
+			// we explicitly choose to not implement it on the root mount,
+			// since this is likely very expensive (and not incredibly useful)
+			contentBasedCache = false
+		}
+
+		deps[m.Input].ContentBasedHash = contentBasedCache
 	}
 	return deps, nil
 }