Merge pull request 'Make an unpacked archive exactly the archive (hq issue 220)' (#84) from fix/issue-220-a-bundle-on-disk-is-exactly-the-artifact into main

internal/apply/archive.go

@@ -66,16 +66,10 @@ func applyArchive(ctx context.Context, r *declaration.Archive, previous store.Ap
 		}
 	}
 
-	if err := os.MkdirAll(r.Path, 0o755); err != nil {
-		return out, err
-	}
-	written, err := unpack(body, r.Path)
+	written, err := replaceWith(body, r.Path, r.Owner)
 	if err != nil {
 		return out, err
 	}
-	if err := ownAll(r.Path, r.Owner); err != nil {
-		return out, err
-	}
 	out.Action = "updated"
 	if previous.Wrote == "" {
 		out.Action = "created"
@@ -84,6 +78,63 @@ func applyArchive(ctx context.Context, r *declaration.Archive, previous store.Ap
 	return out, nil
 }
 
+// replaceWith makes the directory exactly the archive (novox/hq issue 220).
+//
+// **The tree on disk is the archive and nothing else.** The digest is the whole identity of what
+// is unpacked here, so a file the previous archive had and this one does not must go. Unpacked over
+// the old tree, it stayed: a bundle rebuilt as one file per entrypoint kept the package directory
+// of the version before, which code could still import, and a fix that removed a file worked on a
+// fresh machine only. So the archive is unpacked into a fresh directory beside the old one, owned,
+// and swapped in by rename. A running process keeps the files it has open, and the old tree is
+// removed only once the new one is in place. A failed unpack leaves the old tree untouched.
+func replaceWith(body []byte, path, owner string) (int, error) {
+	parent := filepath.Dir(path)
+	if err := os.MkdirAll(parent, 0o755); err != nil {
+		return 0, err
+	}
+	fresh := path + ".unpacking"
+	replaced := path + ".replaced"
+	// What an interrupted earlier attempt left beside the directory.
+	for _, leftover := range []string{fresh, replaced} {
+		if err := os.RemoveAll(leftover); err != nil {
+			return 0, err
+		}
+	}
+	if err := os.Mkdir(fresh, 0o755); err != nil {
+		return 0, err
+	}
+	written, err := unpack(body, fresh)
+	if err == nil {
+		err = ownAll(fresh, owner)
+	}
+	if err != nil {
+		os.RemoveAll(fresh)
+		return written, err
+	}
+	hadOne := true
+	if err := os.Rename(path, replaced); err != nil {
+		if !os.IsNotExist(err) {
+			os.RemoveAll(fresh)
+			return written, err
+		}
+		hadOne = false
+	}
+	if err := os.Rename(fresh, path); err != nil {
+		if hadOne {
+			// Put the old tree back rather than leave nothing at the path.
+			os.Rename(replaced, path)
+		}
+		os.RemoveAll(fresh)
+		return written, err
+	}
+	if hadOne {
+		if err := os.RemoveAll(replaced); err != nil {
+			return written, fmt.Errorf("%s is in place, and the tree it replaced could not be removed: %w", path, err)
+		}
+	}
+	return written, nil
+}
+
 func fetch(ctx context.Context, source string) ([]byte, error) {
 	request, err := http.NewRequestWithContext(ctx, http.MethodGet, source, nil)
 	if err != nil {

internal/apply/archive_replace_test.go

@@ -0,0 +1,68 @@
+package apply
+
+import (
+	"context"
+	"os"
+	"testing"
+
+	"github.com/novox/mesh-host/internal/store"
+)
+
+// novox/hq issue 220: the tree on disk is exactly the archive. A file the previous archive had and
+// this one does not is gone, and nothing is left beside the directory.
+func TestAnArchiveReplacesTheTreeItWasUnpackedOver(t *testing.T) {
+	dir := t.TempDir()
+	first, firstDigest := anArchive(t, map[string]string{"index.js": "old", "node_modules/dep/index.js": "dep"})
+	d := declare(t, `{"id":"bundle-tools","type":"archive","source":"`+serving(t, first)+
+		`","digest":"`+firstDigest+`","path":"`+dir+`/tools"}`)
+	_, state, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginCarried, noServices, nil, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	second, secondDigest := anArchive(t, map[string]string{"index.js": "one file"})
+	d = declare(t, `{"id":"bundle-tools","type":"archive","source":"`+serving(t, second)+
+		`","digest":"`+secondDigest+`","path":"`+dir+`/tools"}`)
+	if _, _, err := Apply(context.Background(), archHost(t), d, state, store.OriginCarried, noServices, nil, nil); err != nil {
+		t.Fatal(err)
+	}
+	if got, _ := os.ReadFile(dir + "/tools/index.js"); string(got) != "one file" {
+		t.Fatalf("index.js is %q", got)
+	}
+	if _, err := os.Stat(dir + "/tools/node_modules"); err == nil {
+		t.Fatal("the previous archive's directory is still there")
+	}
+	entries, _ := os.ReadDir(dir)
+	if len(entries) != 1 {
+		names := []string{}
+		for _, e := range entries {
+			names = append(names, e.Name())
+		}
+		t.Fatalf("beside the tree: %v", names)
+	}
+}
+
+// A tree is replaced only by one that unpacked whole: an archive refused halfway leaves the old
+// tree as it was.
+func TestARefusedArchiveLeavesTheTreeItWouldHaveReplaced(t *testing.T) {
+	dir := t.TempDir()
+	first, firstDigest := anArchive(t, map[string]string{"index.js": "old"})
+	d := declare(t, `{"id":"bundle-tools","type":"archive","source":"`+serving(t, first)+
+		`","digest":"`+firstDigest+`","path":"`+dir+`/tools"}`)
+	_, state, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginCarried, noServices, nil, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	bad, badDigest := anArchive(t, map[string]string{"index.js": "new", "../../escaped": "no"})
+	d = declare(t, `{"id":"bundle-tools","type":"archive","source":"`+serving(t, bad)+
+		`","digest":"`+badDigest+`","path":"`+dir+`/tools"}`)
+	if _, _, err := Apply(context.Background(), archHost(t), d, state, store.OriginCarried, noServices, nil, nil); err == nil {
+		t.Fatal("an escaping archive was accepted")
+	}
+	if got, _ := os.ReadFile(dir + "/tools/index.js"); string(got) != "old" {
+		t.Fatalf("index.js is %q after a refused archive", got)
+	}
+	if entries, _ := os.ReadDir(dir); len(entries) != 1 {
+		t.Fatalf("%d entries beside the tree after a refused archive", len(entries))
+	}
+}