apko: make apk cache safer for multi-writers (#1564)

Currently the cached files are `mv` to the final destination. While this
works for a single process of `apko`, if there are multiple `apko`
processes running, an existing file read can be invalidated if the file
is later replaced (with another file with the same content).

This PR changes to leverage symlink instead. Since file reads in Go by
default will follow symlink at file open time, overwriting the symlink
won't invalidate existing reads.

We also check for the existence of the destination file before
attempting to advertise the cached file, and we clean up the cached file
if we didn't create a symlink at all. This is safe because there should
be no way anyone else could read our version of the cache file.

I've tested with both NFS and gcs-fuse FS.

---------

Signed-off-by: Nghia Tran <[email protected]>

Author: tcnghia

pkg/apk/apk/cache.go

@@ -28,6 +28,8 @@ import (
 
 	"go.opentelemetry.io/otel"
 	"golang.org/x/sync/singleflight"
+
+	"chainguard.dev/apko/pkg/paths"
 )
 
 type Cache struct {
@@ -385,10 +387,9 @@ func (t *cacheTransport) retrieveAndSaveFile(ctx context.Context, request *http.
 
 	// Now that we have the file has been written, rename to atomically populate
 	// the cache
-	if err := os.Rename(tmp.Name(), cacheFile); err != nil {
-		return "", fmt.Errorf("unable to populate cache: %w", err)
+	if err := paths.AdvertiseCachedFile(tmp.Name(), cacheFile); err != nil {
+		return "", err
 	}
-
 	return cacheFile, nil
 }
 

pkg/apk/apk/implementation.go

@@ -53,6 +53,7 @@ import (
 	"chainguard.dev/apko/pkg/apk/expandapk"
 	apkfs "chainguard.dev/apko/pkg/apk/fs"
 	"chainguard.dev/apko/pkg/apk/internal/tarfs"
+	"chainguard.dev/apko/pkg/paths"
 
 	"github.com/chainguard-dev/clog"
 )
@@ -967,7 +968,7 @@ func (a *APK) cachePackage(ctx context.Context, pkg InstallablePackage, exp *exp
 	ctlHex := hex.EncodeToString(exp.ControlHash)
 	ctlDst := filepath.Join(cacheDir, ctlHex+".ctl.tar.gz")
 
-	if err := rename(exp.ControlFile, ctlDst); err != nil {
+	if err := paths.AdvertiseCachedFile(exp.ControlFile, ctlDst); err != nil {
 		return nil, err
 	}
 
@@ -976,7 +977,7 @@ func (a *APK) cachePackage(ctx context.Context, pkg InstallablePackage, exp *exp
 	if exp.SignatureFile != "" {
 		sigDst := filepath.Join(cacheDir, ctlHex+".sig.tar.gz")
 
-		if err := rename(exp.SignatureFile, sigDst); err != nil {
+		if err := paths.AdvertiseCachedFile(exp.SignatureFile, sigDst); err != nil {
 			return nil, err
 		}
 
@@ -986,7 +987,7 @@ func (a *APK) cachePackage(ctx context.Context, pkg InstallablePackage, exp *exp
 	datHex := hex.EncodeToString(exp.PackageHash)
 	datDst := filepath.Join(cacheDir, datHex+".dat.tar.gz")
 
-	if err := rename(exp.PackageFile, datDst); err != nil {
+	if err := paths.AdvertiseCachedFile(exp.PackageFile, datDst); err != nil {
 		return nil, err
 	}
 
@@ -998,7 +999,7 @@ func (a *APK) cachePackage(ctx context.Context, pkg InstallablePackage, exp *exp
 
 	tarDst := strings.TrimSuffix(exp.PackageFile, ".gz")
 
-	if err := rename(exp.TarFile, tarDst); err != nil {
+	if err := paths.AdvertiseCachedFile(exp.TarFile, tarDst); err != nil {
 		return nil, err
 	}
 
@@ -1306,11 +1307,15 @@ func (a *APK) installPackage(ctx context.Context, pkg *Package, expanded *expand
 	log := clog.FromContext(ctx)
 	log.Infof("installing %s (%s)", pkg.Name, pkg.Version)
 
+	// We don't want to call `defer expanded.Close()` to to remove tempDir because our
+	// cached files are advertised by symlinks pointing into them.
+	//
+	// This is not a big deal because the temp files if not referred by
+	// a symlink will be cleaned up anyway.
+
 	ctx, span := otel.Tracer("go-apk").Start(ctx, "installPackage", trace.WithAttributes(attribute.String("package", pkg.Name)))
 	defer span.End()
 
-	defer expanded.Close()
-
 	var (
 		err            error
 		installedFiles []tar.Header
@@ -1376,31 +1381,3 @@ func packageRefs(pkgs []*RepositoryPackage) []string {
 	}
 	return names
 }
-
-func rename(src, dst string) error {
-	if err := os.Rename(src, dst); err != nil {
-		return fmt.Errorf("renaming %s: %w", src, err)
-	}
-
-	// This feels dumb but I have a hunch that it's necessary when renaming
-	// a file on gcsfuse. We seem to get a "file not found" error when reading
-	// something immediately after renaming it.
-	if err := flush(dst); err != nil {
-		return fmt.Errorf("flushing %s: %w", dst, err)
-	}
-
-	return nil
-}
-
-func flush(filename string) error {
-	f, err := os.Open(filename)
-	if err != nil {
-		return err
-	}
-
-	if err := f.Sync(); err != nil {
-		return err
-	}
-
-	return f.Close()
-}

pkg/paths/paths.go

@@ -1,6 +1,21 @@
+// Copyright 2025 Chainguard, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
 package paths
 
 import (
+	"errors"
+	"fmt"
 	"os"
 	"path"
 )
@@ -19,3 +34,30 @@ func ResolvePath(p string, includePaths []string) (string, error) {
 	}
 	return "", os.ErrNotExist
 }
+
+// AdvertisedCachedFile will create a symlink at `dst` pointing to `src`.
+//
+// In the case that `dst` already exists, another process had already created the symlink
+// and we can safely move on. We will also perform a best-effort attempt to clean up the
+// unadvertised file at `src`.
+func AdvertiseCachedFile(src, dst string) error {
+	// Check if the destination already exists.
+	if _, err := os.Stat(dst); err == nil {
+		// Since `src` is unadvertised, it is safe to remove it. Ideally we want this to succeeds,
+		// but we don't want to fail a build just because we couldn't clean up. This will be
+		// left for background clean up process based on age.
+		_ = os.Remove(src)
+		return nil
+	}
+	// Create the symlink.
+	if err := os.Symlink(src, dst); err != nil {
+		// Ignore already exists errors. We don't even want to do clean up here even when
+		// the symlink is pointing somewhere elese, to avoid relying too much on file system
+		// remantics/eventual consistency, etc.
+		if errors.Is(err, os.ErrExist) {
+			return nil
+		}
+		return fmt.Errorf("linking (cached) %s to %s: %w", src, dst, err)
+	}
+	return nil
+}

pkg/paths/paths_test.go

@@ -0,0 +1,62 @@
+// Copyright 2025 Chainguard, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package paths
+
+import (
+	"os"
+	"testing"
+)
+
+func TestAdvertiseCachedFile(t *testing.T) {
+	tmpDir := t.TempDir()
+	src1 := tmpDir + "/src1.tmp"
+	src2 := tmpDir + "/src2.tmp"
+	content := "content"
+	if err := os.WriteFile(src1, []byte(content), 0644); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(src2, []byte(content), 0644); err != nil {
+		t.Fatal(err)
+	}
+	dst := tmpDir + "/target"
+	t.Run("dst does not exists", func(t *testing.T) {
+		if err := AdvertiseCachedFile(src1, dst); err != nil {
+			t.Fatal(err)
+		}
+		dstContent, err := os.ReadFile(dst)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if string(dstContent) != content {
+			t.Fatalf("content mismatch: %s != %s", string(dstContent), content)
+		}
+	})
+
+	t.Run("dst exists", func(t *testing.T) {
+		if err := AdvertiseCachedFile(src2, dst); err != nil {
+			t.Fatal(err)
+		}
+		// check the symlink
+		if l, err := os.Readlink(dst); err != nil {
+			t.Fatal(err)
+		} else if l != src1 {
+			t.Fatalf("symlink should stay in tact: %s != %s", l, src2)
+		}
+
+		// check that src2 is removed
+		if _, err := os.Stat(src2); !os.IsNotExist(err) {
+			t.Fatalf("src2 should be removed: %v", err)
+		}
+	})
+}