From 682eb50613fa829aa4f0a41ca4ec469c36d495ae Mon Sep 17 00:00:00 2001 From: Polina Simonenko Date: Tue, 28 Jul 2026 12:41:05 +0200 Subject: [PATCH 01/22] publish initial releases portably and exchange on darwin The initial managed-release link was committed with os.Link on a symlink, which returns EPERM on macOS and BSD, so creating a first local release failed on every non-Linux platform. os.Symlink is itself an atomic create-if-absent commit, so the reserved-symlink dance is unnecessary there. --- internal/app/commit.go | 25 +++++++++++++++++ internal/app/commit_darwin.go | 11 ++++++++ ..._linux_test.go => commit_exchange_test.go} | 10 +++---- internal/app/commit_linux.go | 28 +++---------------- internal/app/commit_other.go | 25 ++++++----------- internal/app/materialize.go | 13 ++++----- 6 files changed, 59 insertions(+), 53 deletions(-) create mode 100644 internal/app/commit.go create mode 100644 internal/app/commit_darwin.go rename internal/app/{commit_linux_test.go => commit_exchange_test.go} (80%) diff --git a/internal/app/commit.go b/internal/app/commit.go new file mode 100644 index 0000000..5c5def7 --- /dev/null +++ b/internal/app/commit.go @@ -0,0 +1,25 @@ +package app + +import ( + "fmt" + "os" +) + +// exchangeReleaseLink atomically swaps output with the temporary symlink, +// leaving the displaced entry at temporary. Reading back what was displaced is +// what makes the switch a compare-and-swap rather than a lost update: a +// concurrent writer's release survives at temporary and is reported instead of +// being silently overwritten. +func exchangeReleaseLink(temporary, output, expectedLink string) (bool, error) { + if err := exchangePaths(temporary, output); err != nil { + return false, err + } + actualLink, readErr := os.Readlink(temporary) + if readErr == nil && actualLink == expectedLink { + return true, nil + } + if readErr != nil { + return true, fmt.Errorf("entry changed concurrently and is preserved at %q: %w", temporary, readErr) + } + return true, fmt.Errorf("entry changed concurrently from %q to %q and is preserved at %q", expectedLink, actualLink, temporary) +} diff --git a/internal/app/commit_darwin.go b/internal/app/commit_darwin.go new file mode 100644 index 0000000..f8c8f81 --- /dev/null +++ b/internal/app/commit_darwin.go @@ -0,0 +1,11 @@ +//go:build darwin + +package app + +import "golang.org/x/sys/unix" + +// exchangePaths swaps two directory entries in one atomic step. RENAME_SWAP is +// the darwin equivalent of Linux's RENAME_EXCHANGE. +func exchangePaths(from, to string) error { + return unix.RenamexNp(from, to, unix.RENAME_SWAP) +} diff --git a/internal/app/commit_linux_test.go b/internal/app/commit_exchange_test.go similarity index 80% rename from internal/app/commit_linux_test.go rename to internal/app/commit_exchange_test.go index 655cd6b..0351551 100644 --- a/internal/app/commit_linux_test.go +++ b/internal/app/commit_exchange_test.go @@ -1,4 +1,4 @@ -//go:build linux +//go:build linux || darwin package app @@ -8,7 +8,7 @@ import ( "testing" ) -func TestCommitReleaseLinkPreservesConcurrentReplacement(t *testing.T) { +func TestExchangeReleaseLinkPreservesConcurrentReplacement(t *testing.T) { directory := t.TempDir() output := filepath.Join(directory, "repository") temporary := filepath.Join(directory, "new-link") @@ -18,7 +18,7 @@ func TestCommitReleaseLinkPreservesConcurrentReplacement(t *testing.T) { if err := os.Symlink("new-release", temporary); err != nil { t.Fatal(err) } - committed, err := commitReleaseLink(temporary, output, "old-release") + committed, err := exchangeReleaseLink(temporary, output, "old-release") if err == nil || !committed { t.Fatal("expected concurrent replacement to fail") } @@ -31,7 +31,7 @@ func TestCommitReleaseLinkPreservesConcurrentReplacement(t *testing.T) { } } -func TestCommitReleaseLinkExchangesExpectedSymlink(t *testing.T) { +func TestExchangeReleaseLinkExchangesExpectedSymlink(t *testing.T) { directory := t.TempDir() output := filepath.Join(directory, "repository") temporary := filepath.Join(directory, "new-link") @@ -41,7 +41,7 @@ func TestCommitReleaseLinkExchangesExpectedSymlink(t *testing.T) { if err := os.Symlink("new-release", temporary); err != nil { t.Fatal(err) } - committed, err := commitReleaseLink(temporary, output, "old-release") + committed, err := exchangeReleaseLink(temporary, output, "old-release") if err != nil { t.Fatal(err) } diff --git a/internal/app/commit_linux.go b/internal/app/commit_linux.go index a7d0291..5a0207d 100644 --- a/internal/app/commit_linux.go +++ b/internal/app/commit_linux.go @@ -2,29 +2,9 @@ package app -import ( - "fmt" - "os" +import "golang.org/x/sys/unix" - "golang.org/x/sys/unix" -) - -func commitReleaseLink(temporary, output, expectedLink string) (bool, error) { - if expectedLink == "" { - if err := unix.Renameat2(unix.AT_FDCWD, temporary, unix.AT_FDCWD, output, unix.RENAME_NOREPLACE); err != nil { - return false, err - } - return true, nil - } - if err := unix.Renameat2(unix.AT_FDCWD, temporary, unix.AT_FDCWD, output, unix.RENAME_EXCHANGE); err != nil { - return false, err - } - actualLink, readErr := os.Readlink(temporary) - if readErr == nil && actualLink == expectedLink { - return true, nil - } - if readErr != nil { - return true, fmt.Errorf("entry changed concurrently and is preserved at %q: %w", temporary, readErr) - } - return true, fmt.Errorf("entry changed concurrently from %q to %q and is preserved at %q", expectedLink, actualLink, temporary) +// exchangePaths swaps two directory entries in one atomic step. +func exchangePaths(from, to string) error { + return unix.Renameat2(unix.AT_FDCWD, from, unix.AT_FDCWD, to, unix.RENAME_EXCHANGE) } diff --git a/internal/app/commit_other.go b/internal/app/commit_other.go index 4cd24b3..81feae1 100644 --- a/internal/app/commit_other.go +++ b/internal/app/commit_other.go @@ -1,21 +1,14 @@ -//go:build !linux +//go:build !linux && !darwin package app -import ( - "fmt" - "os" -) +import "errors" -func commitReleaseLink(temporary, output, expectedLink string) (bool, error) { - if expectedLink != "" { - return false, fmt.Errorf("atomic managed-release replacement is not implemented on this platform") - } - if err := os.Link(temporary, output); err != nil { - return false, err - } - if err := os.Remove(temporary); err != nil { - return true, err - } - return true, nil +// exchangePaths needs an atomic directory-entry exchange. Linux provides it +// through renameat2(RENAME_EXCHANGE) and darwin through +// renamex_np(RENAME_SWAP); platforms without either cannot replace an existing +// managed release without risking a lost update. Creating a first managed +// release stays portable. +func exchangePaths(_, _ string) error { + return errors.New("atomic managed-release replacement is not implemented on this platform") } diff --git a/internal/app/materialize.go b/internal/app/materialize.go index d7e3aa0..0ab511d 100644 --- a/internal/app/materialize.go +++ b/internal/app/materialize.go @@ -310,13 +310,10 @@ func publishRelease(output, staging, expectedLink, expectedTree string) (bool, e if err := os.Symlink(newLink, filepath.Join(control, "current")); err != nil { return false, fmt.Errorf("create current release link: %w", err) } - temporaryName, err := reserveSymlink(parent, "."+filepath.Base(output)+".snailmail-link-*", filepath.Join(filepath.Base(control), "current")) - if err != nil { - return false, err - } - committed, err := commitReleaseLink(temporaryName, output, "") - if err != nil { - return committed, fmt.Errorf("commit initial release link: %w", err) + // os.Symlink is itself the atomic create-if-absent commit: it fails with + // EEXIST rather than replacing an entry another writer published. + if err := os.Symlink(filepath.Join(filepath.Base(control), "current"), output); err != nil { + return false, fmt.Errorf("commit initial release link: %w", err) } if err := syncDirectory(control); err != nil { return true, err @@ -338,7 +335,7 @@ func publishRelease(output, staging, expectedLink, expectedTree string) (bool, e if err != nil { return false, err } - committed, err := commitReleaseLink(temporaryName, filepath.Join(control, "current"), expectedLink) + committed, err := exchangeReleaseLink(temporaryName, filepath.Join(control, "current"), expectedLink) if err != nil { return committed, fmt.Errorf("commit current release: %w", err) } From b74f686e1b54408e2ae3fc4cf780c52b55d1de94 Mon Sep 17 00:00:00 2001 From: Polina Simonenko Date: Tue, 28 Jul 2026 12:43:24 +0200 Subject: [PATCH 02/22] execute the broker snapshot by a path darwin can resolve The hashed broker snapshot was unlinked and executed through /proc/self/fd/3, or /dev/fd/3 outside Linux. Only Linux resolves that path to the open file itself; darwin reopens it by name, so an unlinked snapshot fails to exec with EACCES and private S3 repositories did not work there at all. --- adapters/credential/command/command.go | 48 ++++++++++++++------- adapters/credential/command/retain_linux.go | 16 +++++++ adapters/credential/command/retain_other.go | 13 ++++++ 3 files changed, 62 insertions(+), 15 deletions(-) create mode 100644 adapters/credential/command/retain_linux.go create mode 100644 adapters/credential/command/retain_other.go diff --git a/adapters/credential/command/command.go b/adapters/credential/command/command.go index 764c6ba..7d97344 100644 --- a/adapters/credential/command/command.go +++ b/adapters/credential/command/command.go @@ -24,9 +24,11 @@ const HelperEnvironment = "SNAILMAIL_CREDENTIAL_BROKER" const brokerCommandTimeout = 30 * time.Second type Broker struct { - mutex sync.Mutex - helper *os.File - identity string + mutex sync.Mutex + helper *os.File + directory string + execPath string + identity string } func NewFromEnvironment() (*Broker, error) { @@ -51,15 +53,28 @@ func NewFromEnvironment() (*Broker, error) { _ = source.Close() return nil, errors.New("private read credential broker must be a native executable") } - snapshot, err := os.CreateTemp("", ".snailmail-credential-broker-*") + // The snapshot lives in a private directory so that retaining it by path, + // which darwin requires, is still unreachable by other users. + directory, err := os.MkdirTemp("", ".snailmail-credential-broker-*") if err != nil { _ = source.Close() return nil, errors.New("private read credential broker executable could not be snapshotted") } + if err := os.Chmod(directory, 0o700); err != nil { + _ = source.Close() + _ = os.RemoveAll(directory) + return nil, errors.New("private read credential broker directory could not be protected") + } + snapshot, err := os.CreateTemp(directory, "helper-*") + if err != nil { + _ = source.Close() + _ = os.RemoveAll(directory) + return nil, errors.New("private read credential broker executable could not be snapshotted") + } snapshotName := snapshot.Name() cleanup := func() { _ = snapshot.Close() - _ = os.Remove(snapshotName) + _ = os.RemoveAll(directory) } hash := sha256.New() _, copyErr := io.Copy(io.MultiWriter(snapshot, hash), source) @@ -77,20 +92,21 @@ func NewFromEnvironment() (*Broker, error) { return nil, errors.New("private read credential broker executable could not be prepared") } if err := snapshot.Close(); err != nil { - _ = os.Remove(snapshotName) + _ = os.RemoveAll(directory) return nil, errors.New("private read credential broker executable could not be prepared") } executable, err := os.Open(snapshotName) if err != nil { - _ = os.Remove(snapshotName) + _ = os.RemoveAll(directory) return nil, errors.New("private read credential broker executable could not be prepared") } - if err := os.Remove(snapshotName); err != nil { + execPath, err := retainSnapshot(snapshotName) + if err != nil { _ = executable.Close() - _ = os.Remove(snapshotName) + _ = os.RemoveAll(directory) return nil, errors.New("private read credential broker executable could not be protected") } - broker := &Broker{helper: executable, identity: hex.EncodeToString(hash.Sum(nil))} + broker := &Broker{helper: executable, directory: directory, execPath: execPath, identity: hex.EncodeToString(hash.Sum(nil))} runtime.SetFinalizer(broker, func(value *Broker) { _ = value.Close() }) return broker, nil } @@ -108,6 +124,12 @@ func (broker *Broker) Close() error { } err := broker.helper.Close() broker.helper = nil + if broker.directory != "" { + if removeErr := os.RemoveAll(broker.directory); err == nil { + err = removeErr + } + broker.directory = "" + } return err } @@ -130,13 +152,9 @@ func (broker *Broker) Issue(ctx context.Context, scope host.ReadScope) (host.Bas if err != nil { return nil, err } - helperPath := "/proc/self/fd/3" - if runtime.GOOS == "darwin" { - helperPath = "/dev/fd/3" - } commandCtx, cancel := context.WithTimeout(ctx, brokerCommandTimeout) defer cancel() - command := exec.CommandContext(commandCtx, helperPath) + command := exec.CommandContext(commandCtx, broker.execPath) command.ExtraFiles = []*os.File{broker.helper} command.Stdin = bytes.NewReader(request) command.Env = brokerEnvironment() diff --git a/adapters/credential/command/retain_linux.go b/adapters/credential/command/retain_linux.go new file mode 100644 index 0000000..350d5d7 --- /dev/null +++ b/adapters/credential/command/retain_linux.go @@ -0,0 +1,16 @@ +//go:build linux + +package commandcredential + +import "os" + +// retainSnapshot unlinks the hashed snapshot and returns the path that resolves +// to the retained inode. /proc/self/fd resolves to the open file itself, so the +// executed bytes remain exactly the hashed bytes even though no name refers to +// them any more. +func retainSnapshot(name string) (string, error) { + if err := os.Remove(name); err != nil { + return "", err + } + return "/proc/self/fd/3", nil +} diff --git a/adapters/credential/command/retain_other.go b/adapters/credential/command/retain_other.go new file mode 100644 index 0000000..11c1c3e --- /dev/null +++ b/adapters/credential/command/retain_other.go @@ -0,0 +1,13 @@ +//go:build !linux + +package commandcredential + +// retainSnapshot keeps the hashed snapshot linked and executes it by path. +// Outside Linux, /dev/fd reopens by path rather than resolving to the open +// file, so an unlinked snapshot cannot be executed at all. The snapshot already +// lives in a private 0700 directory with mode 0500, so no other user can reach +// or rewrite it, and the configured helper path is never consulted again — +// which is what keeps the executed bytes equal to the hashed bytes. +func retainSnapshot(name string) (string, error) { + return name, nil +} From 4d27690917d0c59a9560e949a3497e6faf4318b5 Mon Sep 17 00:00:00 2001 From: Polina Simonenko Date: Tue, 28 Jul 2026 12:45:25 +0200 Subject: [PATCH 03/22] stop redacting a constant, enforce the documented passphrase floor redactCredential derived base64(username+":"+password) even with no credential, producing the constant "Og==" and rewriting it throughout unrelated pip output on every public repository. --- formats/deb/inspect.go | 7 ++++--- internal/app/redact_test.go | 25 +++++++++++++++++++++++++ internal/app/verify.go | 7 ++++++- internal/wire/signers.go | 10 +++++++--- 4 files changed, 42 insertions(+), 7 deletions(-) create mode 100644 internal/app/redact_test.go diff --git a/formats/deb/inspect.go b/formats/deb/inspect.go index 1e24aad..fadd47b 100644 --- a/formats/deb/inspect.go +++ b/formats/deb/inspect.go @@ -3,6 +3,7 @@ package deb import ( "archive/tar" "bufio" + "bytes" "compress/gzip" "errors" "fmt" @@ -238,7 +239,7 @@ func compressedTarReader(name string, raw io.Reader, maxMemory uint64) (io.Reade compressed, err := (xz.ReaderConfig{DictCap: int(maxMemory)}).NewReader(raw) return compressed, nil, err case strings.HasSuffix(name, ".zst"): - compressed, err := zstd.NewReader(raw, zstd.WithDecoderMaxMemory(maxMemory)) + compressed, err := zstd.NewReader(raw, zstd.WithDecoderMaxMemory(maxMemory), zstd.WithDecoderConcurrency(1)) if err != nil { return nil, nil, err } @@ -303,7 +304,7 @@ func readControlArchive(name string, raw io.Reader) (map[string]string, error) { } stream = compressed case strings.HasSuffix(name, ".zst"): - compressed, err := zstd.NewReader(raw, zstd.WithDecoderMaxMemory(maxControlArchive)) + compressed, err := zstd.NewReader(raw, zstd.WithDecoderMaxMemory(maxControlArchive), zstd.WithDecoderConcurrency(1)) if err != nil { return nil, fmt.Errorf("open zstd control archive: %w", err) } @@ -358,7 +359,7 @@ func parseControl(content []byte) (map[string]string, error) { } fields := make(map[string]string) seenFields := make(map[string]bool) - scanner := bufio.NewScanner(strings.NewReader(string(content))) + scanner := bufio.NewScanner(bytes.NewReader(content)) scanner.Buffer(make([]byte, 4096), maxControlSize) current := "" for scanner.Scan() { diff --git a/internal/app/redact_test.go b/internal/app/redact_test.go new file mode 100644 index 0000000..cab81d8 --- /dev/null +++ b/internal/app/redact_test.go @@ -0,0 +1,25 @@ +package app + +import ( + "strings" + "testing" +) + +func TestRedactCredentialLeavesPublicOutputIntact(t *testing.T) { + // base64(":") is "Og==" and can appear in output that has nothing to do with + // a credential; a public repository must not have it rewritten. + output := "Downloading wheel Og== from simple/index.html" + if redacted := redactCredential(output, "", ""); redacted != output { + t.Fatalf("public output was rewritten: %q", redacted) + } +} + +func TestRedactCredentialHidesEveryDerivedForm(t *testing.T) { + output := "user=reader pass=top secret escaped=top+secret basic=cmVhZGVyOnRvcCBzZWNyZXQ=" + redacted := redactCredential(output, "reader", "top secret") + for _, secret := range []string{"reader", "top secret", "top+secret", "cmVhZGVyOnRvcCBzZWNyZXQ="} { + if strings.Contains(redacted, secret) { + t.Fatalf("credential form %q survived redaction in %q", secret, redacted) + } + } +} diff --git a/internal/app/verify.go b/internal/app/verify.go index 97a668c..ac7c608 100644 --- a/internal/app/verify.go +++ b/internal/app/verify.go @@ -72,7 +72,7 @@ func verifyRepository(root string) (buildgraph.RepositoryManifest, []domain.Blob if len(manifestBytes) > maxManifestSize { return buildgraph.RepositoryManifest{}, nil, errors.New("repository manifest exceeds 8 MiB") } - decoder := json.NewDecoder(strings.NewReader(string(manifestBytes))) + decoder := json.NewDecoder(bytes.NewReader(manifestBytes)) decoder.DisallowUnknownFields() var manifest buildgraph.RepositoryManifest if err := decoder.Decode(&manifest); err != nil { @@ -451,6 +451,11 @@ func isLoopbackHost(value string) bool { } func redactCredential(value, username, password string) string { + // With no credential every derived form is a constant — base64(":") in + // particular — so redacting them would rewrite unrelated client output. + if username == "" || password == "" { + return value + } secrets := []string{ username, password, url.QueryEscape(username), url.QueryEscape(password), base64.StdEncoding.EncodeToString([]byte(username + ":" + password)), diff --git a/internal/wire/signers.go b/internal/wire/signers.go index 8266851..a690702 100644 --- a/internal/wire/signers.go +++ b/internal/wire/signers.go @@ -1,7 +1,7 @@ package wire import ( - "errors" + "fmt" "os" "path/filepath" @@ -10,6 +10,10 @@ import ( const SigningPassphraseEnvironment = "SNAILMAIL_KEY_PASSPHRASE" +// MinimumSigningPassphraseBytes is the documented floor for a signing key +// passphrase, which should come from a secret manager rather than be typed. +const MinimumSigningPassphraseBytes = 24 + func NewSignerStore() (*filesigner.Store, error) { dataHome := os.Getenv("XDG_DATA_HOME") if dataHome == "" { @@ -21,8 +25,8 @@ func NewSignerStore() (*filesigner.Store, error) { } return filesigner.New(filepath.Join(dataHome, "snailmail", "private-keys"), func() ([]byte, error) { value := os.Getenv(SigningPassphraseEnvironment) - if len(value) < 16 { - return nil, errors.New("signing key passphrase environment must contain at least 16 bytes") + if len(value) < MinimumSigningPassphraseBytes { + return nil, fmt.Errorf("%s must contain at least %d bytes", SigningPassphraseEnvironment, MinimumSigningPassphraseBytes) } return []byte(value), nil }) From ad93323e383c9ff24c3ea60ce0014d923be58fa6 Mon Sep 17 00:00:00 2001 From: Polina Simonenko Date: Tue, 28 Jul 2026 12:47:53 +0200 Subject: [PATCH 04/22] update dependencies, clearing the reachable circl advisory --- go.mod | 8 ++++---- go.sum | 16 ++++++++-------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/go.mod b/go.mod index 372420a..4c893a4 100644 --- a/go.mod +++ b/go.mod @@ -6,7 +6,7 @@ require ( github.com/Masterminds/semver/v3 v3.5.0 github.com/klauspost/compress v1.19.1 github.com/ulikunitz/xz v0.5.16 - golang.org/x/net v0.43.0 + golang.org/x/net v0.57.0 golang.org/x/sys v0.47.0 gopkg.in/yaml.v3 v3.0.1 ) @@ -16,7 +16,7 @@ require ( github.com/aws/aws-sdk-go-v2 v1.43.0 github.com/aws/aws-sdk-go-v2/config v1.32.31 github.com/aws/aws-sdk-go-v2/service/s3 v1.106.0 - github.com/aws/smithy-go v1.27.3 + github.com/aws/smithy-go v1.27.5 github.com/pelletier/go-toml/v2 v2.4.3 ) @@ -35,6 +35,6 @@ require ( github.com/aws/aws-sdk-go-v2/service/sso v1.33.0 // indirect github.com/aws/aws-sdk-go-v2/service/ssooidc v1.38.0 // indirect github.com/aws/aws-sdk-go-v2/service/sts v1.45.0 // indirect - github.com/cloudflare/circl v1.6.2 // indirect - golang.org/x/crypto v0.41.0 // indirect + github.com/cloudflare/circl v1.6.4 // indirect + golang.org/x/crypto v0.54.0 // indirect ) diff --git a/go.sum b/go.sum index 4af1e2d..d21e05e 100644 --- a/go.sum +++ b/go.sum @@ -36,20 +36,20 @@ github.com/aws/aws-sdk-go-v2/service/ssooidc v1.38.0 h1:tC323YV77QdafeBr6LUhLDTs github.com/aws/aws-sdk-go-v2/service/ssooidc v1.38.0/go.mod h1:SfLK1sgviHmbI+MozR9iDwDjL4cdCVZtahsjoR+z7wg= github.com/aws/aws-sdk-go-v2/service/sts v1.45.0 h1:Pd6PNlp4t8PTXxqzstICl52Wsy78vpjFZ7PRUj44mJc= github.com/aws/aws-sdk-go-v2/service/sts v1.45.0/go.mod h1:rmQ0TnHzuLPmabgjPcsywhsSOmaBDgzR4zvDxSPsGdg= -github.com/aws/smithy-go v1.27.3 h1:F3Zb497UhhskkfpJmfkXswyo+t0sh9OTBnIHjogWbVY= -github.com/aws/smithy-go v1.27.3/go.mod h1:YE2RhdIuDbA5E5bTdciG9KrW3+TiEONeUWCqxX9i1Fc= -github.com/cloudflare/circl v1.6.2 h1:hL7VBpHHKzrV5WTfHCaBsgx/HGbBYlgrwvNXEVDYYsQ= -github.com/cloudflare/circl v1.6.2/go.mod h1:2eXP6Qfat4O/Yhh8BznvKnJ+uzEoTQ6jVKJRn81BiS4= +github.com/aws/smithy-go v1.27.5 h1:d1ro7KpYOYwP6m73YFa+Kc/A130VsAdX68SpsJwARMM= +github.com/aws/smithy-go v1.27.5/go.mod h1:YE2RhdIuDbA5E5bTdciG9KrW3+TiEONeUWCqxX9i1Fc= +github.com/cloudflare/circl v1.6.4 h1:pOXuDTCEYyzydgUpQ0CQz3LsinKjiSk6nNP5Lt5K64U= +github.com/cloudflare/circl v1.6.4/go.mod h1:YxarevkLlbaHuWsxG6vmYNWBEsSp4pnp7j+4VljMavY= github.com/klauspost/compress v1.19.1 h1:VsB4HPswih7mmZ8WleSFQ75c/Ui1M4trX5oAsJnhSlk= github.com/klauspost/compress v1.19.1/go.mod h1:cwPg85FWrGar70rWktvGQj8/hthj3wpl0PGDogxkrSQ= github.com/pelletier/go-toml/v2 v2.4.3 h1:GTRvJQutkOSftxIFD5xw9aepkYNuPWmVJpffdDPYVpY= github.com/pelletier/go-toml/v2 v2.4.3/go.mod h1:2gIqNv+qfxSVS7cM2xJQKtLSTLUE9V8t9Stt+h56mCY= github.com/ulikunitz/xz v0.5.16 h1:ld6NyySjx5lowVKwJvMRLnW5nxKX/xnpSiFYZ/Lxur0= github.com/ulikunitz/xz v0.5.16/go.mod h1:H9Rt/W6/Qj27PGauhQc6nfCDy7vHpzsOThBSaYDoEhw= -golang.org/x/crypto v0.41.0 h1:WKYxWedPGCTVVl5+WHSSrOBT0O8lx32+zxmHxijgXp4= -golang.org/x/crypto v0.41.0/go.mod h1:pO5AFd7FA68rFak7rOAGVuygIISepHftHnr8dr6+sUc= -golang.org/x/net v0.43.0 h1:lat02VYK2j4aLzMzecihNvTlJNQUq316m2Mr9rnM6YE= -golang.org/x/net v0.43.0/go.mod h1:vhO1fvI4dGsIjh73sWfUVjj3N7CA9WkKJNQm2svM6Jg= +golang.org/x/crypto v0.54.0 h1:YLIA59K4fiNzHzjnZt2tUJQjQtUWfWbeHBqKtk3eScw= +golang.org/x/crypto v0.54.0/go.mod h1:KWL8ny2AZdGR2cWmzeHrp2azQPGogOv+HeQaVEXC2dk= +golang.org/x/net v0.57.0 h1:K5+3DljvIuDG9/Jv9rvyMywYNFCQ9RSUY6OOTTkT+tE= +golang.org/x/net v0.57.0/go.mod h1:KpXc8iv+r3XplLAG/f7Jsf9RPszJzdR0f58q9vGOuEU= golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= From 8b251d40f3f89c471190c7cd1d0afa1bc2a69a08 Mon Sep 17 00:00:00 2001 From: Polina Simonenko Date: Tue, 28 Jul 2026 12:57:08 +0200 Subject: [PATCH 05/22] resolve authoritative Git state in batches, and parse status safely Validating authoritative files spawned three Git processes per file: rev-parse --show-prefix, rev-parse :, and hash-object. A plan over eleven repositories cost 44 processes, and apply repeats the whole validation inside its per-repository loop. Resolve both sides in one batch each, cat-file --batch-check for the committed blobs and hash-object --stdin-paths for the worktree, and cache the repository prefix per root. The same eleven-repository plan now costs 20 processes and no longer grows with the file count. hash-object --path also applies the clean filters that produced the committed blob, which fixes a real defect: comparing raw worktree bytes reported every authoritative file as changed in any repository that converts line endings, making such a workspace impossible to plan. Hashing the files rather than consulting the index keeps assume-unchanged from hiding a modification, as before. Status output is now read NUL-delimited, so core.quotePath cannot change the path bytes and a rename carries its original path as its own field rather than an ambiguous "old -> new" string that matched no allowlist entry. Neutralise the config that alters output this package parses, using -c overrides so safe.directory keeps working, and sort the replacement keys so a child environment is a deterministic function of its inputs. --- engine/gitattributes_test.go | 66 +++++++ internal/state/git.go | 280 +++++++++++++++++++++++------- internal/state/git_status_test.go | 49 ++++++ 3 files changed, 336 insertions(+), 59 deletions(-) create mode 100644 engine/gitattributes_test.go create mode 100644 internal/state/git_status_test.go diff --git a/engine/gitattributes_test.go b/engine/gitattributes_test.go new file mode 100644 index 0000000..a80e0fc --- /dev/null +++ b/engine/gitattributes_test.go @@ -0,0 +1,66 @@ +package engine + +import ( + "bytes" + "context" + "os" + "os/exec" + "path/filepath" + "testing" +) + +// A repository that converts line endings stores a different blob than the +// bytes on disk. Comparing raw worktree bytes to the committed blob reported +// every authoritative file as changed, which made planning impossible in any +// such workspace; hashing by path applies the same clean filter to both sides. +func TestPlanAcceptsWorkspaceWithLineEndingConversion(t *testing.T) { + root := t.TempDir() + if output, err := exec.Command("git", "-C", root, "init").CombinedOutput(); err != nil { + t.Fatalf("git init: %v: %s", err, output) + } + if output, err := exec.Command("git", "-C", root, "config", "core.autocrlf", "true").CombinedOutput(); err != nil { + t.Fatalf("git config: %v: %s", err, output) + } + if err := os.WriteFile(filepath.Join(root, ".gitattributes"), []byte("*.toml text eol=crlf\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := InitWorkspace(InitWorkspaceRequest{Root: root, Name: "test-workspace"}); err != nil { + t.Fatal(err) + } + if err := SetupRepository(SetupRepositoryRequest{ + Root: root, Name: "pypi", Format: "pypi", Output: filepath.ToSlash(filepath.Join("public", "pypi")), + }); err != nil { + t.Fatal(err) + } + if output, err := exec.Command("git", "-C", root, "add", "--", + ".gitattributes", ".gitignore", "snailmail.toml", "repos").CombinedOutput(); err != nil { + t.Fatalf("git add: %v: %s", err, output) + } + commit := exec.Command("git", "-C", root, + "-c", "user.name=snailmail-test", "-c", "user.email=test@example.invalid", + "commit", "-m", "initialize converted workspace") + if output, err := commit.CombinedOutput(); err != nil { + t.Fatalf("git commit: %v: %s", err, output) + } + // Re-materialise the worktree so Git applies eol=crlf: the committed blob + // keeps LF while the file on disk gains CRLF, which is exactly the state + // that made raw byte comparison report a spurious difference. + for _, name := range []string{"snailmail.toml", filepath.Join("repos", "pypi.lock.toml")} { + if err := os.Remove(filepath.Join(root, name)); err != nil { + t.Fatal(err) + } + } + if output, err := exec.Command("git", "-C", root, "checkout", "--", ".").CombinedOutput(); err != nil { + t.Fatalf("git checkout: %v: %s", err, output) + } + manifest, err := os.ReadFile(filepath.Join(root, "snailmail.toml")) + if err != nil { + t.Fatal(err) + } + if !bytes.Contains(manifest, []byte("\r\n")) { + t.Fatal("worktree manifest was not converted to CRLF, so this case proves nothing") + } + if _, err := PlanWorkspace(context.Background(), PlanWorkspaceRequest{Root: root}); err != nil { + t.Fatalf("planning a line-ending-converting workspace failed: %v", err) + } +} diff --git a/internal/state/git.go b/internal/state/git.go index 72abe06..60e84af 100644 --- a/internal/state/git.go +++ b/internal/state/git.go @@ -11,6 +11,7 @@ import ( "path/filepath" "sort" "strings" + "sync" "syscall" ) @@ -124,19 +125,19 @@ func requireCleanGitContext(ctx context.Context, root string, allowedUntracked m } func validateGitStatusAllowingUntracked(status string, allowedUntracked, authoritative map[string]bool) error { - for _, line := range strings.Split(status, "\n") { - if line == "" { - continue - } - if len(line) < 4 { - return errors.New("cannot parse Git status") - } - name := filepath.ToSlash(strings.TrimSpace(line[3:])) - if strings.HasPrefix(line, "?? ") && allowedUntracked[name] { + entries, err := parseGitStatus(status) + if err != nil { + return err + } + for _, entry := range entries { + if entry.code == "??" && len(entry.paths) == 1 && allowedUntracked[entry.paths[0]] { continue } - if err := validateGitStatus(line, nil, authoritative); err != nil { - return err + for _, name := range entry.paths { + if entry.code == "??" && !authoritative[name] && !isAuthoritativePath(name) { + continue + } + return fmt.Errorf("workspace has uncommitted authoritative or tracked changes at %q", name) } } return nil @@ -545,30 +546,41 @@ func requireAuthoritativeFilesCommitted(root, revision string, authoritative, al return requireAuthoritativeFilesCommittedContext(context.Background(), root, revision, authoritative, allowedChanges) } +// requireAuthoritativeFilesCommittedContext proves that every authoritative +// file is present in revision and identical to the worktree. +// +// Both sides are resolved in one batch each. The worktree side is hashed by Git +// from the file itself rather than through the index, so neither +// assume-unchanged nor skip-worktree can hide a change, and hashing by path +// applies the same .gitattributes and core.autocrlf clean filters that produced +// the committed blob — comparing raw worktree bytes would report every file as +// changed in any repository that converts line endings. func requireAuthoritativeFilesCommittedContext(ctx context.Context, root, revision string, authoritative, allowedChanges map[string]bool) error { names := make([]string, 0, len(authoritative)) for name := range authoritative { - names = append(names, name) + if !allowedChanges[name] { + names = append(names, name) + } } sort.Strings(names) + if len(names) == 0 { + return nil + } + prefix, err := gitPrefix(ctx, root) + if err != nil { + return err + } + treePaths := make([]string, 0, len(names)) for _, name := range names { - if err := ctx.Err(); err != nil { - return err - } - if allowedChanges[name] { - continue - } - treePath, err := workspaceGitPath(root, name) - if err != nil { - return err - } - expected, err := gitOutputContext(ctx, root, "rev-parse", revision+":"+treePath) - if err != nil { - if ctx.Err() != nil { - return ctx.Err() - } - return fmt.Errorf("authoritative state %q is not committed to Git", name) + // hash-object reads its path list as newline-separated text, so a name + // carrying a newline could inject an entry. Authoritative names come from + // validated manifest fields and fixed globs, but a file on disk can still + // be named adversarially. + if strings.ContainsAny(name, "\n\r") { + return fmt.Errorf("authoritative state %q has an unusable name", name) } + // A symlink with the same bytes is not a committed regular file, and a + // content comparison alone would not tell them apart. workspacePath, err := WorkspacePath(root, name) if err != nil { return err @@ -577,40 +589,139 @@ func requireAuthoritativeFilesCommittedContext(ctx context.Context, root, revisi if err != nil || !info.Mode().IsRegular() { return fmt.Errorf("authoritative state %q is not a regular committed file", name) } - content, err := os.ReadFile(workspacePath) - if err != nil { - return err - } - actual, err := gitHashObjectContext(ctx, root, content) - if err != nil { - if ctx.Err() != nil { - return ctx.Err() - } - return err + treePaths = append(treePaths, filepath.ToSlash(filepath.Join(filepath.FromSlash(prefix), filepath.FromSlash(name)))) + } + committed, err := revisionBlobIDs(ctx, root, revision, treePaths) + if err != nil { + return err + } + for index, name := range names { + if committed[index] == "" { + return fmt.Errorf("authoritative state %q is not committed to Git", name) } - if actual != expected { + } + worktree, err := worktreeBlobIDs(ctx, root, treePaths) + if err != nil { + return err + } + for index, name := range names { + if worktree[index] != committed[index] { return fmt.Errorf("authoritative state %q differs from Git", name) } } return nil } -func validateGitStatus(status string, allowedChanges, authoritative map[string]bool) error { - for _, line := range strings.Split(status, "\n") { - if line == "" { - continue +// revisionBlobIDs resolves every tree path in revision through one cat-file +// batch, reporting an empty ID for a path revision does not contain. +func revisionBlobIDs(ctx context.Context, root, revision string, treePaths []string) ([]string, error) { + var query bytes.Buffer + for _, treePath := range treePaths { + query.WriteString(revision + ":" + treePath + "\n") + } + command := gitCommandContext(ctx, root, "cat-file", "--batch-check") + command.Stdin = &query + output, err := command.Output() + if err != nil { + if ctx.Err() != nil { + return nil, ctx.Err() } - if len(line) < 4 { - return errors.New("cannot parse Git status") + return nil, fmt.Errorf("resolve committed authoritative state: %w", err) + } + lines := strings.Split(strings.TrimRight(string(output), "\n"), "\n") + if len(lines) != len(treePaths) { + return nil, errors.New("cannot resolve committed authoritative state") + } + identifiers := make([]string, len(treePaths)) + for index, line := range lines { + // A resolvable object answers " "; anything else, such + // as " missing", means revision does not contain that path. + fields := strings.Fields(line) + if len(fields) == 3 && fields[1] == "blob" { + identifiers[index] = fields[0] } - name := filepath.ToSlash(strings.TrimSpace(line[3:])) - if allowedChanges[name] { - continue + } + return identifiers, nil +} + +// worktreeBlobIDs hashes each worktree file as Git would when committing it, +// in one batch. Reading the files rather than the index is what keeps +// assume-unchanged and skip-worktree from hiding a modification. Unlike most +// commands, hash-object resolves --stdin-paths entries against the repository +// root rather than the working directory, so these must be tree paths. +func worktreeBlobIDs(ctx context.Context, root string, treePaths []string) ([]string, error) { + var query bytes.Buffer + for _, treePath := range treePaths { + query.WriteString(treePath) + query.WriteString("\n") + } + command := gitCommandContext(ctx, root, "hash-object", "--stdin-paths") + command.Stdin = &query + output, err := command.Output() + if err != nil { + if ctx.Err() != nil { + return nil, ctx.Err() } - if strings.HasPrefix(line, "?? ") && !authoritative[name] && !isAuthoritativePath(name) { + return nil, fmt.Errorf("hash authoritative worktree state: %w", err) + } + lines := strings.Split(strings.TrimRight(string(output), "\n"), "\n") + if len(lines) != len(treePaths) { + return nil, errors.New("cannot hash authoritative worktree state") + } + for index := range lines { + lines[index] = strings.TrimSpace(lines[index]) + } + return lines, nil +} + +type gitStatusEntry struct { + code string + paths []string +} + +// parseGitStatus reads NUL-delimited porcelain v1 records. The NUL format is +// what makes paths trustworthy: it never quotes or escapes, so core.quotePath +// cannot change the bytes, and a rename carries its original path as a separate +// NUL-terminated field instead of an ambiguous "old -> new" string. +func parseGitStatus(status string) ([]gitStatusEntry, error) { + records := strings.Split(status, "\x00") + var entries []gitStatusEntry + for index := 0; index < len(records); index++ { + record := records[index] + if record == "" { continue } - return fmt.Errorf("workspace has uncommitted authoritative or tracked changes at %q", name) + if len(record) < 4 { + return nil, errors.New("cannot parse Git status") + } + entry := gitStatusEntry{code: record[:2], paths: []string{filepath.ToSlash(record[3:])}} + if entry.code[0] == 'R' || entry.code[0] == 'C' || entry.code[1] == 'R' || entry.code[1] == 'C' { + index++ + if index >= len(records) { + return nil, errors.New("cannot parse Git status") + } + entry.paths = append(entry.paths, filepath.ToSlash(records[index])) + } + entries = append(entries, entry) + } + return entries, nil +} + +func validateGitStatus(status string, allowedChanges, authoritative map[string]bool) error { + entries, err := parseGitStatus(status) + if err != nil { + return err + } + for _, entry := range entries { + for _, name := range entry.paths { + if allowedChanges[name] { + continue + } + if entry.code == "??" && !authoritative[name] && !isAuthoritativePath(name) { + continue + } + return fmt.Errorf("workspace has uncommitted authoritative or tracked changes at %q", name) + } } return nil } @@ -699,8 +810,38 @@ func publicationWorkspacePaths(repositories []string) map[string]bool { return paths } +// gitPrefix returns the workspace's path prefix inside its Git repository. +// The prefix cannot change while a workspace operation holds its lock, and it +// is consulted once per authoritative path, so it is resolved once per root +// rather than through a subprocess per lookup. +var gitPrefixes struct { + sync.Mutex + byRoot map[string]string +} + +func gitPrefix(ctx context.Context, root string) (string, error) { + gitPrefixes.Lock() + cached, found := gitPrefixes.byRoot[root] + gitPrefixes.Unlock() + if found { + return cached, nil + } + prefix, err := gitOutputContext(ctx, root, "rev-parse", "--show-prefix") + if err != nil { + return "", err + } + prefix = filepath.ToSlash(prefix) + gitPrefixes.Lock() + if gitPrefixes.byRoot == nil { + gitPrefixes.byRoot = make(map[string]string) + } + gitPrefixes.byRoot[root] = prefix + gitPrefixes.Unlock() + return prefix, nil +} + func workspaceGitPath(root, relative string) (string, error) { - prefix, err := gitOutput(root, "rev-parse", "--show-prefix") + prefix, err := gitPrefix(context.Background(), root) if err != nil { return "", err } @@ -708,11 +849,10 @@ func workspaceGitPath(root, relative string) (string, error) { } func workspaceRelativeGitPath(root, name string) (string, bool, error) { - prefix, err := gitOutput(root, "rev-parse", "--show-prefix") + prefix, err := gitPrefix(context.Background(), root) if err != nil { return "", false, err } - prefix = filepath.ToSlash(prefix) name = filepath.ToSlash(name) if prefix == "" { return name, true, nil @@ -885,9 +1025,24 @@ func gitCommand(root string, arguments ...string) *exec.Cmd { return gitCommandContext(context.Background(), root, arguments...) } +// gitConfigOverrides neutralise user configuration that would otherwise change +// output this package parses. They are applied as -c overrides rather than by +// disabling global and system configuration wholesale, so that settings the +// repository genuinely needs — safe.directory in particular, which Git accepts +// only from those files — keep working. +var gitConfigOverrides = []string{ + // Prepends signature verification output to `log --format=%B`. + "-c", "log.showSignature=false", + // Can answer from a stale daemon view of the worktree. + "-c", "core.fsmonitor=", + // Quoting would change the path bytes read back from status output. + "-c", "core.quotePath=false", +} + func gitCommandContext(ctx context.Context, root string, arguments ...string) *exec.Cmd { - command := exec.CommandContext(ctx, "git", append([]string{"-C", root}, arguments...)...) - command.Env = replaceEnvironment(os.Environ(), "GIT_OPTIONAL_LOCKS", "0") + full := append([]string{"-C", root}, gitConfigOverrides...) + command := exec.CommandContext(ctx, "git", append(full, arguments...)...) + command.Env = replaceEnvironment(os.Environ(), "GIT_OPTIONAL_LOCKS", "0", "GIT_TERMINAL_PROMPT", "0") return command } @@ -912,11 +1067,11 @@ func gitStatusOutput(root string) (string, error) { } func gitStatusOutputContext(ctx context.Context, root string) (string, error) { - output, err := gitCommandContext(ctx, root, "status", "--porcelain", "--untracked-files=all").Output() + output, err := gitCommandContext(ctx, root, "status", "--porcelain=v1", "-z", "--untracked-files=all", "--no-renames").Output() if err != nil { return "", err } - return strings.TrimRight(string(output), "\r\n"), nil + return string(output), nil } func gitOutputEnv(root string, environment []string, arguments ...string) (string, error) { @@ -952,8 +1107,15 @@ func replaceEnvironment(environment []string, values ...string) []string { } result = append(result, entry) } - for key, value := range replacements { - result = append(result, key+"="+value) + // Map iteration order is random; sort so the child environment is a + // deterministic function of its inputs. + keys := make([]string, 0, len(replacements)) + for key := range replacements { + keys = append(keys, key) + } + sort.Strings(keys) + for _, key := range keys { + result = append(result, key+"="+replacements[key]) } return result } diff --git a/internal/state/git_status_test.go b/internal/state/git_status_test.go new file mode 100644 index 0000000..f3d9411 --- /dev/null +++ b/internal/state/git_status_test.go @@ -0,0 +1,49 @@ +package state + +import "testing" + +func TestParseGitStatusReadsRenameAndSpacedPaths(t *testing.T) { + // Porcelain v1 -z emits a rename as the new path followed by the original + // path in its own NUL-terminated field, and never quotes or escapes. + status := "R repos/new name.lock.toml\x00repos/old name.lock.toml\x00 M snailmail.toml\x00?? notes.txt\x00" + entries, err := parseGitStatus(status) + if err != nil { + t.Fatal(err) + } + if len(entries) != 3 { + t.Fatalf("parsed %d entries, want 3: %+v", len(entries), entries) + } + rename := entries[0] + if rename.code != "R " || len(rename.paths) != 2 || + rename.paths[0] != "repos/new name.lock.toml" || rename.paths[1] != "repos/old name.lock.toml" { + t.Fatalf("rename entry parsed as %+v", rename) + } + if entries[1].code != " M" || entries[1].paths[0] != "snailmail.toml" { + t.Fatalf("modification entry parsed as %+v", entries[1]) + } + if entries[2].code != "??" || entries[2].paths[0] != "notes.txt" { + t.Fatalf("untracked entry parsed as %+v", entries[2]) + } +} + +func TestValidateGitStatusRejectsRenamedAuthoritativeState(t *testing.T) { + // The old newline parser produced the single name "a -> b", which matched no + // allowlist entry and no authoritative path, so a renamed lock could slip + // through as an unrecognised entry. + status := "R repos/renamed.lock.toml\x00repos/pypi.lock.toml\x00" + authoritative := map[string]bool{"repos/pypi.lock.toml": true} + if err := validateGitStatus(status, nil, authoritative); err == nil { + t.Fatal("expected a renamed authoritative lock to be rejected") + } +} + +func TestValidateGitStatusAllowsPermittedUntrackedPath(t *testing.T) { + status := "?? publications/pypi.jsonl\x00" + allowed := map[string]bool{"publications/pypi.jsonl": true} + if err := validateGitStatusAllowingUntracked(status, allowed, nil); err != nil { + t.Fatalf("permitted untracked ledger was rejected: %v", err) + } + if err := validateGitStatusAllowingUntracked(status, nil, nil); err == nil { + t.Fatal("expected an unpermitted untracked ledger to be rejected") + } +} From e69ce44e4d2cfe6c57da62f098eb13ed7feb166c Mon Sep 17 00:00:00 2001 From: Polina Simonenko Date: Tue, 28 Jul 2026 13:02:34 +0200 Subject: [PATCH 06/22] replay publication history from HEAD through one cat-file batch The replay walked every ref. A ledger version that only ever existed on an abandoned branch or an old tag was folded into the immutable-record set and the current worktree ledger was then reported as having removed it, so an unrelated stale branch made planning impossible. Scope the walk to HEAD, which is the line of history the records belong to. It also spawned a `git show` per historical commit, per repository, on add, plan, apply and check, so the cost grew without bound as a workspace accumulated publications. Read every version through one cat-file batch and parse each distinct blob once. Naming a revision fails on an unborn HEAD where a whole-repository walk was simply empty, so a workspace that records artifacts before its first commit now reports no history rather than an error. --- internal/state/git.go | 83 +++++++++++++++++++++++++++ internal/state/ledger.go | 43 +++++++++++--- internal/state/ledger_history_test.go | 83 +++++++++++++++++++++++++++ 3 files changed, 200 insertions(+), 9 deletions(-) create mode 100644 internal/state/ledger_history_test.go diff --git a/internal/state/git.go b/internal/state/git.go index 60e84af..c27c508 100644 --- a/internal/state/git.go +++ b/internal/state/git.go @@ -1,6 +1,7 @@ package state import ( + "bufio" "bytes" "context" "errors" @@ -10,6 +11,7 @@ import ( "os/exec" "path/filepath" "sort" + "strconv" "strings" "sync" "syscall" @@ -644,6 +646,87 @@ func revisionBlobIDs(ctx context.Context, root, revision string, treePaths []str return identifiers, nil } +// maxBatchObjectSize bounds a single object read through catFileBatch. Ledger +// blobs are the only use, and they are line-oriented records. +const maxBatchObjectSize = 64 << 20 + +type batchObject struct { + id string + content []byte +} + +// catFileBatch reads every requested revspec through one cat-file process +// rather than one `git show` per object. Objects the repository does not +// contain come back with an empty id. +func catFileBatch(ctx context.Context, root string, revspecs []string) ([]batchObject, error) { + if len(revspecs) == 0 { + return nil, nil + } + var query bytes.Buffer + for _, revspec := range revspecs { + query.WriteString(revspec) + query.WriteString("\n") + } + command := gitCommandContext(ctx, root, "cat-file", "--batch") + command.Stdin = &query + output, err := command.StdoutPipe() + if err != nil { + return nil, err + } + if err := command.Start(); err != nil { + return nil, err + } + objects, readErr := readCatFileBatch(bufio.NewReaderSize(output, 64<<10), len(revspecs)) + if readErr != nil { + _ = command.Process.Kill() + _ = command.Wait() + if ctx.Err() != nil { + return nil, ctx.Err() + } + return nil, readErr + } + if err := command.Wait(); err != nil { + if ctx.Err() != nil { + return nil, ctx.Err() + } + return nil, fmt.Errorf("read Git objects: %w", err) + } + return objects, nil +} + +func readCatFileBatch(reader *bufio.Reader, expected int) ([]batchObject, error) { + objects := make([]batchObject, 0, expected) + for len(objects) < expected { + header, err := reader.ReadString('\n') + if err != nil { + return nil, fmt.Errorf("read Git object header: %w", err) + } + fields := strings.Fields(strings.TrimSuffix(header, "\n")) + // " missing" for anything the repository does not contain. + if len(fields) == 2 && fields[1] == "missing" { + objects = append(objects, batchObject{}) + continue + } + if len(fields) != 3 || fields[1] != "blob" { + return nil, errors.New("unexpected Git object header") + } + size, err := strconv.ParseInt(fields[2], 10, 64) + if err != nil || size < 0 || size > maxBatchObjectSize { + return nil, errors.New("Git object size is invalid or exceeds the read limit") + } + content := make([]byte, size) + if _, err := io.ReadFull(reader, content); err != nil { + return nil, fmt.Errorf("read Git object: %w", err) + } + // cat-file --batch terminates each object with a newline. + if _, err := reader.Discard(1); err != nil { + return nil, fmt.Errorf("read Git object terminator: %w", err) + } + objects = append(objects, batchObject{id: fields[0], content: content}) + } + return objects, nil +} + // worktreeBlobIDs hashes each worktree file as Git would when committing it, // in one batch. Reading the files rather than the index is what keeps // assume-unchanged and skip-worktree from hiding a modification. Unlike most diff --git a/internal/state/ledger.go b/internal/state/ledger.go index 7359be0..15bfb41 100644 --- a/internal/state/ledger.go +++ b/internal/state/ledger.go @@ -38,8 +38,13 @@ func LoadLedgerHistory(root, repository string) ([]PublicationRecord, error) { return LoadLedgerHistoryContext(context.Background(), root, repository) } +// LoadLedgerHistoryContext replays the publication history reachable from HEAD. +// Scoping to the current history is deliberate: a record that only ever existed +// on an abandoned branch or an old tag was never part of this line of history, +// and folding those in reported perfectly good ledgers as having "removed an +// immutable record". func LoadLedgerHistoryContext(ctx context.Context, root, repository string) ([]PublicationRecord, error) { - return loadLedgerHistoryContext(ctx, root, repository, "--all") + return loadLedgerHistoryContext(ctx, root, repository, "HEAD") } func LoadLedgerHistoryAtRevisionContext(ctx context.Context, root, repository, revision string) ([]PublicationRecord, error) { @@ -75,20 +80,40 @@ func loadLedgerHistoryContext(ctx context.Context, root, repository, revision st if ctx.Err() != nil { return nil, ctx.Err() } + // A workspace can record artifacts before its first commit, and naming a + // revision fails outright on an unborn HEAD where a whole-repository walk + // would simply have been empty. + if _, verifyErr := gitOutputContext(ctx, root, "rev-parse", "--verify", "--quiet", revision+"^{commit}"); verifyErr != nil { + return records, nil + } + return nil, fmt.Errorf("read publication history: %w", err) + } + commits := strings.Fields(output) + revspecs := make([]string, 0, len(commits)) + for _, commit := range commits { + revspecs = append(revspecs, commit+":"+treePath) + } + // One cat-file batch instead of a `git show` process per historical commit, + // which grew without bound as a workspace accumulated publications. + objects, err := catFileBatch(ctx, root, revspecs) + if err != nil { return nil, fmt.Errorf("read publication history: %w", err) } - for _, revision := range strings.Fields(output) { + replayed := make(map[string]bool, len(objects)) + for index, object := range objects { if err := ctx.Err(); err != nil { return nil, err } - content, err := execGitShowContext(ctx, root, revision, treePath) - if err != nil { - if ctx.Err() != nil { - return nil, ctx.Err() - } - return nil, fmt.Errorf("read publication ledger at %s: %w", revision, err) + if object.id == "" { + return nil, fmt.Errorf("read publication ledger at %s", commits[index]) + } + // Successive commits often carry identical ledger bytes; parsing each + // distinct blob once keeps the replay linear in content, not commits. + if replayed[object.id] { + continue } - historical, err := parseLedger(bytes.NewReader(content)) + replayed[object.id] = true + historical, err := parseLedger(bytes.NewReader(object.content)) if err != nil { return nil, err } diff --git a/internal/state/ledger_history_test.go b/internal/state/ledger_history_test.go new file mode 100644 index 0000000..14c7dd9 --- /dev/null +++ b/internal/state/ledger_history_test.go @@ -0,0 +1,83 @@ +package state + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "testing" +) + +func runGit(t *testing.T, root string, arguments ...string) { + t.Helper() + full := append([]string{"-C", root, + "-c", "user.name=snailmail-test", "-c", "user.email=test@example.invalid"}, arguments...) + if output, err := exec.Command("git", full...).CombinedOutput(); err != nil { + t.Fatalf("git %v: %v: %s", arguments, err, output) + } +} + +func writeLedger(t *testing.T, root, repository, content string) { + t.Helper() + directory := filepath.Join(root, "publications") + if err := os.MkdirAll(directory, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(directory, repository+".jsonl"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} + +func ledgerLine(planID, treeSHA, pkg, version string) string { + return `{"schema_version":1,"plan_id":"` + planID + `","change_id":"pypi:` + treeSHA[:12] + + `","repository":"pypi","package":"` + pkg + `","version":"` + version + + `","blob_sha256":["` + treeSHA + `"],"tree_sha256":"` + treeSHA + `","recorded_at":"2026-01-01T00:00:00Z"}` + "\n" +} + +// A record that only ever existed on an abandoned branch was never part of this +// line of history. Replaying every ref folded it in and then reported the +// perfectly good worktree ledger as having removed an immutable record. +func TestLedgerHistoryIgnoresAbandonedBranch(t *testing.T) { + root := t.TempDir() + runGit(t, root, "init") + planID := "11" + "00000000000000000000000000000000000000000000000000000000000000"[:62] + treeSHA := "22" + "00000000000000000000000000000000000000000000000000000000000000"[:62] + kept := ledgerLine(planID, treeSHA, "kept", "1.0.0") + writeLedger(t, root, "pypi", kept) + runGit(t, root, "add", "-A") + runGit(t, root, "commit", "-m", "record kept publication") + + runGit(t, root, "checkout", "-q", "-b", "abandoned") + otherPlan := "33" + "00000000000000000000000000000000000000000000000000000000000000"[:62] + otherTree := "44" + "00000000000000000000000000000000000000000000000000000000000000"[:62] + writeLedger(t, root, "pypi", kept+ledgerLine(otherPlan, otherTree, "abandoned", "9.9.9")) + runGit(t, root, "add", "-A") + runGit(t, root, "commit", "-m", "record abandoned publication") + + runGit(t, root, "checkout", "-q", "-") + writeLedger(t, root, "pypi", kept) + + records, err := LoadLedgerHistoryContext(context.Background(), root, "pypi") + if err != nil { + t.Fatalf("history replay rejected a workspace with an abandoned branch: %v", err) + } + if len(records) != 1 || records[0].Package != "kept" { + t.Fatalf("replayed %d records, want only the reachable one: %+v", len(records), records) + } +} + +// Records committed on the current line of history are still immutable. +func TestLedgerHistoryRejectsRemovedReachableRecord(t *testing.T) { + root := t.TempDir() + runGit(t, root, "init") + planID := "55" + "00000000000000000000000000000000000000000000000000000000000000"[:62] + treeSHA := "66" + "00000000000000000000000000000000000000000000000000000000000000"[:62] + writeLedger(t, root, "pypi", ledgerLine(planID, treeSHA, "kept", "1.0.0")) + runGit(t, root, "add", "-A") + runGit(t, root, "commit", "-m", "record publication") + + writeLedger(t, root, "pypi", "") + if _, err := LoadLedgerHistoryContext(context.Background(), root, "pypi"); err == nil { + t.Fatal("expected removal of a reachable immutable record to be rejected") + } +} From 83dae5b0c2a7a5264f57682529b0bfd1e05e0025 Mon Sep 17 00:00:00 2001 From: Polina Simonenko Date: Tue, 28 Jul 2026 13:06:45 +0200 Subject: [PATCH 07/22] remove dead code and stop wiping secrets into immutable strings --- adapters/credential/command/command.go | 19 ++++++++++++------- engine/workspace.go | 26 +++++++------------------- engine/workspace_test.go | 22 ++++++++++++++++++++++ internal/state/cas.go | 8 -------- internal/state/git.go | 4 ---- internal/state/ledger.go | 20 -------------------- internal/state/ledger_test.go | 6 ++++++ internal/state/store.go | 19 +++++++++++++++++-- 8 files changed, 64 insertions(+), 60 deletions(-) diff --git a/adapters/credential/command/command.go b/adapters/credential/command/command.go index 7d97344..4897182 100644 --- a/adapters/credential/command/command.go +++ b/adapters/credential/command/command.go @@ -183,7 +183,7 @@ func (broker *Broker) Issue(ctx context.Context, scope host.ReadScope) (host.Bas if err != nil || !expiresAt.After(time.Now().UTC()) || expiresAt.After(time.Now().UTC().Add(15*time.Minute)) || response.Username == "" || response.Password == "" { return nil, errors.New("credential broker returned invalid or excessive credentials") } - return &credential{username: response.Username, password: response.Password, expiresAt: expiresAt}, nil + return &credential{username: []byte(response.Username), password: []byte(response.Password), expiresAt: expiresAt}, nil } func brokerEnvironment() []string { @@ -230,27 +230,32 @@ func (buffer *limitedBuffer) Write(content []byte) (int, error) { return len(content), nil } +// credential holds its secret as bytes so Destroy can actually overwrite it. +// Go strings are immutable, so assigning "" only drops a reference and leaves +// the secret readable in the heap until the collector happens to reuse it. type credential struct { mutex sync.Mutex - username string - password string + username []byte + password []byte expiresAt time.Time } func (credential *credential) Basic(_ context.Context) (string, string, error) { credential.mutex.Lock() defer credential.mutex.Unlock() - if credential.username == "" || credential.password == "" || !time.Now().UTC().Before(credential.expiresAt) { + if len(credential.username) == 0 || len(credential.password) == 0 || !time.Now().UTC().Before(credential.expiresAt) { return "", "", errors.New("private read credential is unavailable or expired") } - return credential.username, credential.password, nil + return string(credential.username), string(credential.password), nil } func (credential *credential) Destroy() { credential.mutex.Lock() defer credential.mutex.Unlock() - credential.username = "" - credential.password = "" + clear(credential.username) + clear(credential.password) + credential.username = nil + credential.password = nil credential.expiresAt = time.Time{} } diff --git a/engine/workspace.go b/engine/workspace.go index 0f3d6c6..c70fd3a 100644 --- a/engine/workspace.go +++ b/engine/workspace.go @@ -1121,6 +1121,10 @@ func ApplyWorkspace(ctx context.Context, request ApplyWorkspaceRequest) (ApplyWo return result, err } if committed.Access.Credential != nil { + // The deferred destroy covers the error paths below, which return + // immediately. Successful iterations destroy their own credential at + // the end so that a multi-repository apply does not accumulate live + // short-lived credentials until the whole apply finishes. defer committed.Access.Credential.Destroy() } result.Applied++ @@ -1170,6 +1174,9 @@ func ApplyWorkspace(ctx context.Context, request ApplyWorkspaceRequest) (ApplyWo return result, err } deployments = append(deployments, deploymentRecordFor(item.planned, item.deployment, item.observed, item.signingState, committed.Revision.NativeRevision, plan.PlanID, plan.Payload.CreatedAt, request.currentTime())) + if committed.Access.Credential != nil { + committed.Access.Credential.Destroy() + } } if unlockGit != nil { unlockGit() @@ -1554,25 +1561,6 @@ func materializeLockedArtifact(ctx context.Context, output string, generatedAt t return BuildResult{Format: manifest.Format, Output: output, TreeSHA256: manifest.TreeSHA256, ManifestSHA256: manifestSHA256}, nil } -func activePackageVersions(lock state.RepositoryLock) []state.PackageVersion { - active := make(map[string]bool) - for _, placement := range lock.Placement { - active[placement.Package+"\x00"+placement.Version] = true - } - var result []state.PackageVersion - for _, packageVersion := range lock.PackageVersion { - if active[packageVersion.Package+"\x00"+packageVersion.Version] { - result = append(result, packageVersion) - } - } - return result -} - -func activeLock(lock state.RepositoryLock) state.RepositoryLock { - lock.PackageVersion = activePackageVersions(lock) - return lock -} - func visiblePackageVersions(lock state.RepositoryLock, repository state.Repository) []state.PackageVersion { active := make(map[string]bool) for _, placement := range lock.Placement { diff --git a/engine/workspace_test.go b/engine/workspace_test.go index 06c622b..d116f6d 100644 --- a/engine/workspace_test.go +++ b/engine/workspace_test.go @@ -1981,3 +1981,25 @@ func (remote *recordingHost) Restore(context.Context, host.Repository, host.Rest func (remote *recordingHost) Abort(context.Context, host.Repository, host.StagedPublication) error { return nil } + +// Publication-record tests need the lock restricted to placed versions +// regardless of a repository's rendered view; production code always filters +// through a repository, so these live with their only callers. +func activeLock(lock state.RepositoryLock) state.RepositoryLock { + lock.PackageVersion = activePackageVersions(lock) + return lock +} + +func activePackageVersions(lock state.RepositoryLock) []state.PackageVersion { + active := make(map[string]bool) + for _, placement := range lock.Placement { + active[placement.Package+"\x00"+placement.Version] = true + } + var result []state.PackageVersion + for _, packageVersion := range lock.PackageVersion { + if active[packageVersion.Package+"\x00"+packageVersion.Version] { + result = append(result, packageVersion) + } + } + return result +} diff --git a/internal/state/cas.go b/internal/state/cas.go index 02b8297..d4f06ab 100644 --- a/internal/state/cas.go +++ b/internal/state/cas.go @@ -222,10 +222,6 @@ func LoadBlobContext(ctx context.Context, root, format string, locked LockedBlob return result, name, nil } -func ValidateLockedBlobFile(name, format string, locked LockedBlob) (domain.Blob, error) { - return validateLockedBlob(name, format, locked) -} - func ValidateLockedBlobReference(format string, locked LockedBlob) error { decoded, err := hex.DecodeString(locked.SHA256) if err != nil || len(decoded) != sha256.Size || locked.SHA256 != strings.ToLower(locked.SHA256) { @@ -241,10 +237,6 @@ func ValidateLockedBlobReference(format string, locked LockedBlob) error { return nil } -func ValidateLockedBlobOpen(file *os.File, info os.FileInfo, name, format string, locked LockedBlob) (domain.Blob, error) { - return validateLockedBlobOpen(file, info, name, format, locked) -} - func ValidateLockedBlobOpenContext(ctx context.Context, file *os.File, info os.FileInfo, name, format string, locked LockedBlob) (domain.Blob, error) { return validateLockedBlobOpenContext(ctx, file, info, name, format, locked) } diff --git a/internal/state/git.go b/internal/state/git.go index c27c508..65149e9 100644 --- a/internal/state/git.go +++ b/internal/state/git.go @@ -54,10 +54,6 @@ func RequireGitRepositoryContext(ctx context.Context, root string) error { return nil } -func RequireCompleteGitHistory(root string) error { - return requireCompleteGitHistoryContext(context.Background(), root) -} - func RequireCompleteGitHistoryContext(ctx context.Context, root string) error { return requireCompleteGitHistoryContext(ctx, root) } diff --git a/internal/state/ledger.go b/internal/state/ledger.go index 15bfb41..242f81d 100644 --- a/internal/state/ledger.go +++ b/internal/state/ledger.go @@ -167,10 +167,6 @@ func parseLedger(reader io.Reader) ([]PublicationRecord, error) { return records, nil } -func recordIdentity(record PublicationRecord) string { - return record.Repository + "\x00" + record.PlanID + "\x00" + record.ChangeID + "\x00" + record.Package + "\x00" + record.Version + "\x00" + strings.Join(record.BlobSHA256, ",") + "\x00" + record.TreeSHA256 + "\x00" + record.RecordedAt -} - func publicationRecordKey(record PublicationRecord) string { return record.PlanID + "\x00" + record.ChangeID + "\x00" + record.Package + "\x00" + record.Version } @@ -309,22 +305,6 @@ func PreparePublicationRecords(root, baseRevision, repository, planID, changeID, return atomicWrite(name, expectedContent, 0o644) } -func ValidatePreparedPublicationLedger(root, baseRevision, repository, planID, changeID, treeSHA, recordedAt string, lock RepositoryLock) error { - expectedContent, err := expectedPublicationLedger(root, baseRevision, repository, planID, changeID, treeSHA, recordedAt, lock) - if err != nil { - return err - } - name, err := ledgerPath(root, repository) - if err != nil { - return err - } - actualContent, err := os.ReadFile(name) - if err != nil { - return err - } - return comparePublicationLedger(repository, expectedContent, actualContent) -} - func ValidateCommittedPublicationLedger(root, baseRevision, committedRevision, repository, planID, changeID, treeSHA, recordedAt string, lock RepositoryLock) error { expectedContent, err := expectedPublicationLedger(root, baseRevision, repository, planID, changeID, treeSHA, recordedAt, lock) if err != nil { diff --git a/internal/state/ledger_test.go b/internal/state/ledger_test.go index ab7661d..8ce9187 100644 --- a/internal/state/ledger_test.go +++ b/internal/state/ledger_test.go @@ -97,3 +97,9 @@ func TestPublicationRecordIdentityIncludesAuditFields(t *testing.T) { t.Fatal("publication identity ignored its audit timestamp") } } + +// recordIdentity distinguishes two records by every field a ledger line +// carries; it exists for these assertions, not for production use. +func recordIdentity(record PublicationRecord) string { + return record.Repository + "\x00" + record.PlanID + "\x00" + record.ChangeID + "\x00" + record.Package + "\x00" + record.Version + "\x00" + strings.Join(record.BlobSHA256, ",") + "\x00" + record.TreeSHA256 + "\x00" + record.RecordedAt +} diff --git a/internal/state/store.go b/internal/state/store.go index 1e549d5..55f8cdd 100644 --- a/internal/state/store.go +++ b/internal/state/store.go @@ -868,7 +868,9 @@ func AddBlob(lock *RepositoryLock, format, track, distro string, blob LockedBlob if !placementExists { lock.Placement = append(lock.Placement, Placement{Package: packageName, Version: version, Track: track, Distro: distro}) } - canonicalizeLock(lock) + // Canonical order is a property of the written file, and WriteLock + // establishes it. Sorting here instead re-sorted the whole lock once per + // added artifact, which made `snailmail add` quadratic in an existing lock. return !blobExists || !placementExists, nil } @@ -1243,7 +1245,7 @@ func ensureGitignore(root string) error { if err != nil && !errors.Is(err, os.ErrNotExist) { return err } - if strings.Contains(string(content), ".snailmail/") { + if hasIgnoreRule(content, ".snailmail/") { return nil } if len(content) != 0 && content[len(content)-1] != '\n' { @@ -1252,3 +1254,16 @@ func ensureGitignore(root string) error { content = append(content, []byte(block)...) return atomicWrite(name, content, 0o644) } + +// hasIgnoreRule reports whether rule is present as its own directive. A +// substring search also matched the rule inside a comment or, worse, inside a +// "!.snailmail/" negation that re-includes exactly what must stay ignored. +func hasIgnoreRule(content []byte, rule string) bool { + for _, line := range strings.Split(string(content), "\n") { + line = strings.TrimSpace(strings.TrimSuffix(line, "\r")) + if line == rule || line == "/"+rule { + return true + } + } + return false +} From 2a6c095281383f7dab1059336c3275d6cbc3502b Mon Sep 17 00:00:00 2001 From: Polina Simonenko Date: Tue, 28 Jul 2026 13:09:23 +0200 Subject: [PATCH 08/22] stream host-served verification and render Packages once --- formats/deb/render.go | 19 +++++++++-------- formats/deb/render_bench_test.go | 36 ++++++++++++++++++++++++++++++++ internal/app/verify.go | 25 ++++++++++++---------- 3 files changed, 60 insertions(+), 20 deletions(-) create mode 100644 formats/deb/render_bench_test.go diff --git a/formats/deb/render.go b/formats/deb/render.go index ad9e845..04f3644 100644 --- a/formats/deb/render.go +++ b/formats/deb/render.go @@ -118,14 +118,17 @@ func Build(blobs []domain.Blob, options BuildOptions) (domain.RepositoryArtifact var releaseFiles []releaseFile for _, architecture := range architectures { - var packages strings.Builder + // Render straight into the buffer whose bytes become the file. Building a + // string per stanza, copying it into an index builder and then copying + // the whole index again to bytes held a large Packages index three times. + var packages bytes.Buffer for _, entry := range indexed { if entry.blob.Facts.Architecture != "all" && entry.blob.Facts.Architecture != architecture { continue } - packages.WriteString(renderPackage(entry)) + writePackage(&packages, entry) } - packagesBytes := []byte(packages.String()) + packagesBytes := packages.Bytes() base := path.Join("dists", options.Suite, options.Component, "binary-"+architecture) packagesPath := path.Join(base, "Packages") files = append(files, domain.File{Path: packagesPath, Content: packagesBytes}) @@ -210,7 +213,7 @@ func packagePoolPath(component string, blob domain.Blob) string { return path.Join("pool", component, prefix, blob.Facts.Name, blob.Filename) } -func renderPackage(entry indexedBlob) string { +func writePackage(output *bytes.Buffer, entry indexedBlob) { fields := make(map[string]string, len(entry.blob.Facts.Fields)+5) for name, value := range entry.blob.Facts.Fields { if name != "Status" && name != "Filename" && name != "Size" && name != "MD5sum" && name != "SHA1" && name != "SHA256" { @@ -234,12 +237,10 @@ func renderPackage(entry indexedBlob) string { } return names[i] < names[j] }) - var stanza strings.Builder for _, name := range names { - writeField(&stanza, name, fields[name]) + writeField(output, name, fields[name]) } - stanza.WriteByte('\n') - return stanza.String() + output.WriteByte('\n') } func fieldRank(name string) int { @@ -252,7 +253,7 @@ func fieldRank(name string) int { return len(order) } -func writeField(output *strings.Builder, name, value string) { +func writeField(output *bytes.Buffer, name, value string) { lines := strings.Split(value, "\n") output.WriteString(name) output.WriteString(": ") diff --git a/formats/deb/render_bench_test.go b/formats/deb/render_bench_test.go new file mode 100644 index 0000000..ce67669 --- /dev/null +++ b/formats/deb/render_bench_test.go @@ -0,0 +1,36 @@ +package deb + +import ( + "fmt" + "testing" + "time" + + "github.com/shellcell/snailmail/internal/domain" +) + +// BenchmarkBuildPackagesIndex guards the memory cost of rendering a large +// Packages index, which is the largest allocation in a Debian build. +func BenchmarkBuildPackagesIndex(b *testing.B) { + blobs := make([]domain.Blob, 0, 2000) + for i := 0; i < 2000; i++ { + name := fmt.Sprintf("pkg%04d", i) + blobs = append(blobs, domain.Blob{ + Filename: name + "_1.0.0_amd64.deb", Size: 1024, + MD5: "0123456789abcdef0123456789abcdef", + SHA1: "0123456789abcdef0123456789abcdef01234567", + SHA256: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", + Facts: domain.PackageFacts{Name: name, Version: "1.0.0", Architecture: "amd64", Fields: map[string]string{ + "Package": name, "Version": "1.0.0", "Architecture": "amd64", + "Description": "a package\nwith a long multi-line description\n.\nand more text to make the stanza realistic", + "Maintainer": "Someone ", "Depends": "libc6, libssl3, zlib1g", + }}, + }) + } + options := BuildOptions{Suite: "stable", Component: "main", Architectures: []string{"amd64"}, GeneratedAt: time.Unix(0, 0).UTC()} + b.ReportAllocs() + for b.Loop() { + if _, err := Build(blobs, options); err != nil { + b.Fatal(err) + } + } +} diff --git a/internal/app/verify.go b/internal/app/verify.go index ac7c608..e0e8f31 100644 --- a/internal/app/verify.go +++ b/internal/app/verify.go @@ -374,38 +374,41 @@ func verifyHostedPyPIBytes(ctx context.Context, access host.ClientAccess, userna return errors.New("repository verification does not follow redirects") }, } - verifyURL := func(address string, expectedSize int64, expectedSHA256 string) ([]byte, error) { + // Routes cover every file in the tree, including distributions up to + // pypi.MaxArtifactSize. Only the digest and length matter here, so the body + // is hashed as it streams rather than buffered whole. + verifyURL := func(address string, expectedSize int64, expectedSHA256 string) error { request, err := http.NewRequestWithContext(ctx, http.MethodGet, address, nil) if err != nil { - return nil, err + return err } if private { request.SetBasicAuth(username, password) } response, err := client.Do(request) if err != nil { - return nil, err + return err } - content, readErr := io.ReadAll(io.LimitReader(response.Body, expectedSize+1)) + hash := sha256.New() + size, readErr := io.Copy(hash, io.LimitReader(response.Body, expectedSize+1)) closeErr := response.Body.Close() if readErr != nil { - return nil, readErr + return readErr } if closeErr != nil { - return nil, closeErr + return closeErr } - digest := sha256.Sum256(content) - if response.StatusCode != http.StatusOK || int64(len(content)) != expectedSize || hex.EncodeToString(digest[:]) != expectedSHA256 { - return nil, errors.New("host-served repository bytes do not match the reviewed tree") + if response.StatusCode != http.StatusOK || size != expectedSize || hex.EncodeToString(hash.Sum(nil)) != expectedSHA256 { + return errors.New("host-served repository bytes do not match the reviewed tree") } - return content, nil + return nil } for _, route := range access.Routes { parsed, err := url.Parse(route.URL) if err != nil || parsed.Scheme != endpoint.Scheme || parsed.Host != endpoint.Host || parsed.User != nil || parsed.RawQuery != "" || parsed.Fragment != "" || route.Size < 0 { return errors.New("host supplied an invalid client verification route") } - if _, err := verifyURL(route.URL, route.Size, route.SHA256); err != nil { + if err := verifyURL(route.URL, route.Size, route.SHA256); err != nil { return fmt.Errorf("verify host-served route %q: %w", parsed.EscapedPath(), err) } } From 9a062629f35248e9d096fc18da84b695b19a630d Mon Sep 17 00:00:00 2001 From: Polina Simonenko Date: Tue, 28 Jul 2026 13:12:36 +0200 Subject: [PATCH 09/22] stage inside the workspace and scope the container runner environment --- engine/staging_test.go | 64 +++++++++++++++++++++++++++++++++++++ engine/workspace.go | 38 ++++++++++++++++++++-- internal/app/environment.go | 39 ++++++++++++++++++++++ internal/app/verify.go | 4 +-- 4 files changed, 140 insertions(+), 5 deletions(-) create mode 100644 engine/staging_test.go create mode 100644 internal/app/environment.go diff --git a/engine/staging_test.go b/engine/staging_test.go new file mode 100644 index 0000000..372e839 --- /dev/null +++ b/engine/staging_test.go @@ -0,0 +1,64 @@ +package engine + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" +) + +// Build and stage trees must not land in TMPDIR: it is commonly a tmpfs, so a +// large repository would be held in RAM, and it is a different filesystem from +// the local CAS, so linking artifacts into a build input degrades to copying +// every byte. +func TestPlanStagesInsideWorkspaceRatherThanTempDir(t *testing.T) { + root := t.TempDir() + isolatedTemp := t.TempDir() + t.Setenv("TMPDIR", isolatedTemp) + + initializeRepository(t, root, "pypi") + artifact := workspaceArtifact(t, root, "pypi", "1.2.3") + if _, err := AddArtifacts(AddArtifactsRequest{Root: root, Repository: "pypi", Artifacts: []string{artifact}}); err != nil { + t.Fatal(err) + } + commitWorkspace(t, root, "initialize workspace") + if _, err := PlanWorkspace(context.Background(), PlanWorkspaceRequest{Root: root}); err != nil { + t.Fatal(err) + } + + entries, err := os.ReadDir(isolatedTemp) + if err != nil { + t.Fatal(err) + } + for _, entry := range entries { + if strings.Contains(entry.Name(), "snailmail") { + t.Fatalf("planning left %q in TMPDIR", entry.Name()) + } + } + if _, err := os.Lstat(filepath.Join(root, ".snailmail", "stage")); err != nil { + t.Fatalf("workspace staging directory was not created: %v", err) + } +} + +func TestStagingRootRejectsRelativeOverride(t *testing.T) { + t.Setenv(StageDirectoryEnvironment, "relative/stage") + if _, err := stagingRoot(t.TempDir()); err == nil { + t.Fatal("expected a relative staging override to be rejected") + } +} + +func TestStagingRootHonorsAbsoluteOverride(t *testing.T) { + override := filepath.Join(t.TempDir(), "elsewhere") + t.Setenv(StageDirectoryEnvironment, override) + directory, err := stagingRoot(t.TempDir()) + if err != nil { + t.Fatal(err) + } + if directory != override { + t.Fatalf("staging root = %q, want %q", directory, override) + } + if _, err := os.Lstat(override); err != nil { + t.Fatalf("override directory was not created: %v", err) + } +} diff --git a/engine/workspace.go b/engine/workspace.go index c70fd3a..42a8d48 100644 --- a/engine/workspace.go +++ b/engine/workspace.go @@ -869,7 +869,11 @@ func ApplyWorkspace(ctx context.Context, request ApplyWorkspaceRequest) (ApplyWo prepared = append(prepared, item) continue } - stage, err := os.MkdirTemp("", ".snailmail-apply-*") + staging, err := stagingRoot(root) + if err != nil { + return ApplyWorkspaceResult{}, err + } + stage, err := os.MkdirTemp(staging, ".snailmail-apply-*") if err != nil { return ApplyWorkspaceResult{}, err } @@ -1452,7 +1456,11 @@ func verifyCanonicalClient(ctx context.Context, root string, repository state.Re } func buildLockedRepository(ctx context.Context, root, name string, repository state.Repository, lock state.RepositoryLock, generatedAt, signatureTime time.Time, output string, blobStore blob.Store, keys map[string]state.SigningKey, plannedSigning []state.PlanSigning, signers signer.Resolver) (BuildResult, error) { - input, err := os.MkdirTemp("", ".snailmail-locked-input-*") + staging, err := stagingRoot(root) + if err != nil { + return BuildResult{}, err + } + input, err := os.MkdirTemp(staging, ".snailmail-locked-input-*") if err != nil { return BuildResult{}, err } @@ -1480,7 +1488,7 @@ func buildLockedRepository(ctx context.Context, root, name string, repository st } } if output == "" { - temporary, err := os.MkdirTemp("", ".snailmail-plan-build-*") + temporary, err := os.MkdirTemp(staging, ".snailmail-plan-build-*") if err != nil { return BuildResult{}, err } @@ -1697,6 +1705,30 @@ func linkOrCopy(source, target string) error { return closeInputErr } +// StageDirectoryEnvironment overrides where build and stage trees are created. +const StageDirectoryEnvironment = "SNAILMAIL_STAGE_DIR" + +// stagingRoot returns the directory that holds temporary build and stage trees. +// +// These default to the workspace rather than TMPDIR for two reasons. TMPDIR is +// commonly a tmpfs, so a multi-gigabyte repository tree would be held in RAM, +// and it is a different filesystem from the local CAS, so linking artifacts +// into a build input falls back to copying every byte. Staging beside the CAS +// keeps the links and the bytes on disk. +func stagingRoot(root string) (string, error) { + directory := filepath.Join(root, ".snailmail", "stage") + if override := strings.TrimSpace(os.Getenv(StageDirectoryEnvironment)); override != "" { + if !filepath.IsAbs(override) { + return "", fmt.Errorf("%s must be an absolute path", StageDirectoryEnvironment) + } + directory = override + } + if err := os.MkdirAll(directory, 0o700); err != nil { + return "", fmt.Errorf("create staging directory: %w", err) + } + return directory, nil +} + func workspaceRoot(root string) (string, error) { if root == "" { root = "." diff --git a/internal/app/environment.go b/internal/app/environment.go new file mode 100644 index 0000000..5da2f84 --- /dev/null +++ b/internal/app/environment.go @@ -0,0 +1,39 @@ +package app + +import ( + "os" + "sort" + "strings" +) + +// runnerEnvironmentAllowlist is what a container runner legitimately needs to +// find its socket, configuration and credentials. Everything else — the signing +// passphrase and cloud credentials in particular — has no business reaching a +// process whose only job is to run a verification image. +var runnerEnvironmentAllowlist = map[string]bool{ + "PATH": true, "HOME": true, "TMPDIR": true, "USER": true, "LOGNAME": true, + "LANG": true, "LC_ALL": true, "TERM": true, + "XDG_RUNTIME_DIR": true, "XDG_CONFIG_HOME": true, "XDG_DATA_HOME": true, + "SSL_CERT_FILE": true, "SSL_CERT_DIR": true, + "DOCKER_HOST": true, "DOCKER_CONFIG": true, "DOCKER_CERT_PATH": true, "DOCKER_TLS_VERIFY": true, + "CONTAINER_HOST": true, "CONTAINER_CONNECTION": true, "CONTAINERS_CONF": true, + "CONTAINERS_STORAGE_CONF": true, "CONTAINERS_REGISTRIES_CONF": true, + "REGISTRY_AUTH_FILE": true, "BUILDAH_ISOLATION": true, + "HTTP_PROXY": true, "HTTPS_PROXY": true, "NO_PROXY": true, + "http_proxy": true, "https_proxy": true, "no_proxy": true, +} + +// runnerEnvironment returns the allowlisted subset of the process environment, +// in sorted order so a child environment is a deterministic function of its +// inputs. +func runnerEnvironment() []string { + environment := make([]string, 0, len(runnerEnvironmentAllowlist)) + for _, entry := range os.Environ() { + name, _, found := strings.Cut(entry, "=") + if found && runnerEnvironmentAllowlist[name] { + environment = append(environment, entry) + } + } + sort.Strings(environment) + return environment +} diff --git a/internal/app/verify.go b/internal/app/verify.go index e0e8f31..3cb863d 100644 --- a/internal/app/verify.go +++ b/internal/app/verify.go @@ -575,7 +575,7 @@ fi `, } command := exec.CommandContext(caseCtx, runner, arguments...) - command.Env = os.Environ() + command.Env = runnerEnvironment() output, err := command.CombinedOutput() if err != nil { return fmt.Errorf("Debian client verification for %s=%s/%s: %w\n%s", verification.Package, verification.Version, verification.Architecture, err, strings.TrimSpace(string(output))) @@ -752,7 +752,7 @@ fi `, } command := exec.CommandContext(caseCtx, runner, arguments...) - command.Env = os.Environ() + command.Env = runnerEnvironment() output, err := command.CombinedOutput() if err != nil { return fmt.Errorf("Helm client verification for %s@%s: %w\n%s", verification.Project, verification.Version, err, strings.TrimSpace(string(output))) From 2176863733b04936f9d7705f82af9ed0f1d5debb Mon Sep 17 00:00:00 2001 From: Polina Simonenko Date: Tue, 28 Jul 2026 13:14:58 +0200 Subject: [PATCH 10/22] allow building without the AWS SDK Building with -tags nos3 drops the stripped binary from 20.6 MB to 12.8 MB. The tag is opt-out so the default build and the shipped image keep S3; excluded builds report that rather than failing obscurely. --- README.md | 11 +++++++++++ adapters/blob/s3/aws.go | 2 ++ adapters/blob/s3/aws_disabled.go | 16 ++++++++++++++++ adapters/blob/s3/aws_test.go | 2 ++ adapters/host/s3/aws.go | 2 ++ adapters/host/s3/aws_disabled.go | 16 ++++++++++++++++ adapters/host/s3/aws_test.go | 2 ++ 7 files changed, 51 insertions(+) create mode 100644 adapters/blob/s3/aws_disabled.go create mode 100644 adapters/host/s3/aws_disabled.go diff --git a/README.md b/README.md index 6dbd1c5..089cfab 100644 --- a/README.md +++ b/README.md @@ -285,6 +285,17 @@ locks, ledgers, deployment receipts, and an optional current plan: go run ./cmd/snailmail render --output site ``` +The AWS SDK is the largest single component of the binary. Builds that only +target local directories or GitHub Pages can exclude it: + +```sh +go build -trimpath -ldflags="-s -w" -tags nos3 ./cmd/snailmail +``` + +That drops the stripped binary from about 20.6 MB to 12.8 MB. S3 hosts and S3 +blob stores then report that the build has no S3 support; every other format, +host, and command is unaffected. The default build keeps S3. + `Dockerfile` builds the runtime image; `.github/workflows/snailmail.yml` is a pinned template with read-only PR testing and a protected default-branch apply job. Configure `AWS_ROLE_ARN`/`AWS_REGION` repository variables for OIDC-backed diff --git a/adapters/blob/s3/aws.go b/adapters/blob/s3/aws.go index 5b1097a..c836cf9 100644 --- a/adapters/blob/s3/aws.go +++ b/adapters/blob/s3/aws.go @@ -1,3 +1,5 @@ +//go:build !nos3 + package s3blob import ( diff --git a/adapters/blob/s3/aws_disabled.go b/adapters/blob/s3/aws_disabled.go new file mode 100644 index 0000000..80c5abb --- /dev/null +++ b/adapters/blob/s3/aws_disabled.go @@ -0,0 +1,16 @@ +//go:build nos3 + +package s3blob + +import ( + "context" + "errors" + + "github.com/shellcell/snailmail/blob" +) + +// NewAWS reports that this build excludes S3 support. The protocol logic in +// this package is SDK-free and still compiled; only the AWS client is absent. +func NewAWS(_ context.Context, _ blob.Configuration) (*Store, error) { + return nil, errors.New("this snailmail build was compiled without S3 support (nos3)") +} diff --git a/adapters/blob/s3/aws_test.go b/adapters/blob/s3/aws_test.go index 22f160f..492d88a 100644 --- a/adapters/blob/s3/aws_test.go +++ b/adapters/blob/s3/aws_test.go @@ -1,3 +1,5 @@ +//go:build !nos3 + package s3blob import ( diff --git a/adapters/host/s3/aws.go b/adapters/host/s3/aws.go index bc74da1..b595e0f 100644 --- a/adapters/host/s3/aws.go +++ b/adapters/host/s3/aws.go @@ -1,3 +1,5 @@ +//go:build !nos3 + package s3host import ( diff --git a/adapters/host/s3/aws_disabled.go b/adapters/host/s3/aws_disabled.go new file mode 100644 index 0000000..ff4c110 --- /dev/null +++ b/adapters/host/s3/aws_disabled.go @@ -0,0 +1,16 @@ +//go:build nos3 + +package s3host + +import ( + "context" + "errors" + + "github.com/shellcell/snailmail/host" +) + +// NewAWS reports that this build excludes S3 support. The protocol logic in +// this package is SDK-free and still compiled; only the AWS client is absent. +func NewAWS(_ context.Context, _ host.Repository, _ ...host.CredentialBroker) (*Adapter, error) { + return nil, errors.New("this snailmail build was compiled without S3 support (nos3)") +} diff --git a/adapters/host/s3/aws_test.go b/adapters/host/s3/aws_test.go index 02b0368..b57e986 100644 --- a/adapters/host/s3/aws_test.go +++ b/adapters/host/s3/aws_test.go @@ -1,3 +1,5 @@ +//go:build !nos3 + package s3host import ( From d6145ee2457fb81f34a56d235ffc0fe97356db4e Mon Sep 17 00:00:00 2001 From: Polina Simonenko Date: Tue, 28 Jul 2026 13:18:00 +0200 Subject: [PATCH 11/22] issue S3 object requests concurrently --- adapters/host/s3/parallel.go | 71 ++++++++++++++++++++ adapters/host/s3/parallel_test.go | 103 ++++++++++++++++++++++++++++++ adapters/host/s3/s3.go | 53 +++++++++------ 3 files changed, 208 insertions(+), 19 deletions(-) create mode 100644 adapters/host/s3/parallel.go create mode 100644 adapters/host/s3/parallel_test.go diff --git a/adapters/host/s3/parallel.go b/adapters/host/s3/parallel.go new file mode 100644 index 0000000..4b7a583 --- /dev/null +++ b/adapters/host/s3/parallel.go @@ -0,0 +1,71 @@ +package s3host + +import ( + "context" + "runtime" + "sync" +) + +// defaultObjectConcurrency bounds in-flight object requests. Publication is +// dominated by round-trip latency rather than local work, so this is +// deliberately higher than the CPU count, but still low enough to stay well +// inside per-connection limits and to keep a failure from fanning out widely. +var defaultObjectConcurrency = min(16, max(4, runtime.GOMAXPROCS(0)*4)) + +// forEachObject runs work over every index in [0, count) with bounded +// concurrency and returns the error belonging to the lowest index, so that a +// failure is reported identically no matter which request lost the race. +func forEachObject(ctx context.Context, count int, work func(ctx context.Context, index int) error) error { + if count == 0 { + return nil + } + if count == 1 { + return work(ctx, 0) + } + workers := min(defaultObjectConcurrency, count) + // A cancelled context stops the remaining work as soon as one request fails. + groupCtx, cancel := context.WithCancel(ctx) + defer cancel() + + errs := make([]error, count) + next := make(chan int) + var waitGroup sync.WaitGroup + waitGroup.Add(workers) + for range workers { + go func() { + defer waitGroup.Done() + for index := range next { + if groupCtx.Err() != nil { + return + } + if err := work(groupCtx, index); err != nil { + errs[index] = err + cancel() + return + } + } + }() + } + for index := range count { + select { + case next <- index: + case <-groupCtx.Done(): + close(next) + waitGroup.Wait() + return firstError(errs, ctx) + } + } + close(next) + waitGroup.Wait() + return firstError(errs, ctx) +} + +func firstError(errs []error, ctx context.Context) error { + for _, err := range errs { + if err != nil { + return err + } + } + // No worker recorded a failure, so any stop came from the caller's context. + return ctx.Err() +} diff --git a/adapters/host/s3/parallel_test.go b/adapters/host/s3/parallel_test.go new file mode 100644 index 0000000..a99e681 --- /dev/null +++ b/adapters/host/s3/parallel_test.go @@ -0,0 +1,103 @@ +package s3host + +import ( + "context" + "errors" + "fmt" + "sync" + "sync/atomic" + "testing" +) + +func TestForEachObjectRunsEveryIndexOnce(t *testing.T) { + const count = 500 + var mutex sync.Mutex + seen := make(map[int]int, count) + if err := forEachObject(context.Background(), count, func(_ context.Context, index int) error { + mutex.Lock() + defer mutex.Unlock() + seen[index]++ + return nil + }); err != nil { + t.Fatal(err) + } + if len(seen) != count { + t.Fatalf("ran %d indices, want %d", len(seen), count) + } + for index, times := range seen { + if times != 1 { + t.Fatalf("index %d ran %d times", index, times) + } + } +} + +func TestForEachObjectActuallyOverlaps(t *testing.T) { + var inFlight, peak atomic.Int64 + if err := forEachObject(context.Background(), 64, func(_ context.Context, _ int) error { + current := inFlight.Add(1) + for { + previous := peak.Load() + if current <= previous || peak.CompareAndSwap(previous, current) { + break + } + } + // Hold the slot so other workers have a chance to overlap. + for i := 0; i < 20000; i++ { + _ = i + } + inFlight.Add(-1) + return nil + }); err != nil { + t.Fatal(err) + } + if peak.Load() < 2 { + t.Fatalf("peak concurrency was %d; work did not overlap", peak.Load()) + } +} + +// Whichever request loses the race, the reported failure must be the same one, +// otherwise an operator sees a different error for the same broken state. +func TestForEachObjectReportsLowestFailingIndex(t *testing.T) { + for attempt := range 50 { + err := forEachObject(context.Background(), 200, func(_ context.Context, index int) error { + if index == 7 || index == 90 || index == 150 { + return fmt.Errorf("object %d failed", index) + } + return nil + }) + if err == nil || err.Error() != "object 7 failed" { + t.Fatalf("attempt %d reported %v, want the lowest failing index", attempt, err) + } + } +} + +func TestForEachObjectStopsOnCancelledContext(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + var ran atomic.Int64 + err := forEachObject(ctx, 100, func(_ context.Context, _ int) error { + ran.Add(1) + return nil + }) + if !errors.Is(err, context.Canceled) { + t.Fatalf("error = %v, want context.Canceled", err) + } + if ran.Load() == 100 { + t.Fatal("cancellation did not stop the remaining work") + } +} + +func TestForEachObjectHandlesEmptyAndSingle(t *testing.T) { + if err := forEachObject(context.Background(), 0, func(context.Context, int) error { + t.Fatal("empty work ran") + return nil + }); err != nil { + t.Fatal(err) + } + sentinel := errors.New("single") + if err := forEachObject(context.Background(), 1, func(context.Context, int) error { + return sentinel + }); !errors.Is(err, sentinel) { + t.Fatalf("error = %v, want the sentinel", err) + } +} diff --git a/adapters/host/s3/s3.go b/adapters/host/s3/s3.go index 1a23d59..eca63b6 100644 --- a/adapters/host/s3/s3.go +++ b/adapters/host/s3/s3.go @@ -141,17 +141,21 @@ func (adapter *Adapter) Observe(ctx context.Context, repository host.Repository) return host.PublishedRevision{}, &host.Error{Kind: host.ErrorIndeterminate, Operation: "observe S3 restore state", Err: errors.New("bound retained root is corrupt")} } } - for _, file := range descriptor.Files { + if err := forEachObject(ctx, len(descriptor.Files), func(ctx context.Context, index int) error { + file := descriptor.Files[index] info, err := adapter.client.Head(ctx, releaseKey(repository, revision.TreeSHA256, file.Path)) if errors.Is(err, ErrNotFound) { - return host.PublishedRevision{}, &host.Error{Kind: host.ErrorIndeterminate, Operation: "observe S3 release", Err: fmt.Errorf("release object %q is missing or corrupt", file.Path)} + return &host.Error{Kind: host.ErrorIndeterminate, Operation: "observe S3 release", Err: fmt.Errorf("release object %q is missing or corrupt", file.Path)} } if err != nil { - return host.PublishedRevision{}, infrastructure("inspect S3 release object", err) + return infrastructure("inspect S3 release object", err) } if info.Size != file.Size || info.SHA256 != file.SHA256 || info.Metadata["sha256"] != file.SHA256 { - return host.PublishedRevision{}, &host.Error{Kind: host.ErrorIndeterminate, Operation: "observe S3 release", Err: fmt.Errorf("release object %q is missing or corrupt", file.Path)} + return &host.Error{Kind: host.ErrorIndeterminate, Operation: "observe S3 release", Err: fmt.Errorf("release object %q is missing or corrupt", file.Path)} } + return nil + }); err != nil { + return host.PublishedRevision{}, err } releaseRoot, _, err := adapter.client.Get(ctx, releaseKey(repository, revision.TreeSHA256, pypiRootPath), maximumMetadataSize) if err != nil { @@ -264,35 +268,42 @@ func (adapter *Adapter) Stage(ctx context.Context, repository host.Repository, r resultErr = fmt.Errorf("%w; clean partial S3 stage: %v", resultErr, cleanupErr) } }() + // Every key is recorded before any upload starts so the deferred cleanup + // removes partial stages regardless of which uploads were in flight. for _, file := range descriptor.Files { + uploaded = append(uploaded, stageKey(repository, identifier, file.Path)) + } + if err := forEachObject(ctx, len(descriptor.Files), func(ctx context.Context, index int) error { + file := descriptor.Files[index] name := filepath.Join(request.Directory, filepath.FromSlash(file.Path)) actualSize, actualDigest, err := hashFile(name) if err != nil { - return host.StagedPublication{}, err + return err } if actualSize != file.Size || actualDigest != file.SHA256 { - return host.StagedPublication{}, &host.Error{Kind: host.ErrorStale, Operation: "stage S3 repository", Err: fmt.Errorf("file %q changed after verification", file.Path)} + return &host.Error{Kind: host.ErrorStale, Operation: "stage S3 repository", Err: fmt.Errorf("file %q changed after verification", file.Path)} } input, err := os.Open(name) if err != nil { - return host.StagedPublication{}, err + return err } - key := stageKey(repository, identifier, file.Path) - uploaded = append(uploaded, key) stored, putErr := adapter.client.Put(ctx, PutRequest{ - Key: key, Body: input, Size: file.Size, SHA256: file.SHA256, + Key: stageKey(repository, identifier, file.Path), Body: input, Size: file.Size, SHA256: file.SHA256, ContentType: contentType(file.Path), Metadata: map[string]string{"sha256": file.SHA256}, }) closeErr := input.Close() if putErr != nil { - return host.StagedPublication{}, infrastructure("upload S3 stage object", putErr) + return infrastructure("upload S3 stage object", putErr) } if closeErr != nil { - return host.StagedPublication{}, closeErr + return closeErr } if stored.Size != file.Size || stored.SHA256 != file.SHA256 || stored.Metadata["sha256"] != file.SHA256 { - return host.StagedPublication{}, &host.Error{Kind: host.ErrorInfrastructure, Operation: "verify S3 stage object", Err: fmt.Errorf("object %q metadata does not match", file.Path)} + return &host.Error{Kind: host.ErrorInfrastructure, Operation: "verify S3 stage object", Err: fmt.Errorf("object %q metadata does not match", file.Path)} } + return nil + }); err != nil { + return host.StagedPublication{}, err } manifest, err := app.VerifyRepository(request.Directory) if err != nil || manifest.Format != pypi.FormatID || manifest.TreeSHA256 != request.TreeSHA256 { @@ -699,25 +710,29 @@ func descriptorFromRequest(request host.StageRequest) (publicationDescriptor, st func (adapter *Adapter) materializeRelease(ctx context.Context, repository host.Repository, stageID string, publication publicationDescriptor) (string, error) { descriptor := releaseDescriptorFromPublication(publication) - for _, file := range descriptor.Files { + if err := forEachObject(ctx, len(descriptor.Files), func(ctx context.Context, index int) error { + file := descriptor.Files[index] destination := releaseKey(repository, descriptor.TreeSHA256, file.Path) stored, err := adapter.client.CopyCreate(ctx, stageKey(repository, stageID, file.Path), destination, file.Size, file.SHA256) if errors.Is(err, ErrPrecondition) { stored, err = adapter.client.Head(ctx, destination) if err != nil { - return "", infrastructure("reconcile S3 release object", err) + return infrastructure("reconcile S3 release object", err) } if stored.Size != file.Size || stored.SHA256 != file.SHA256 || stored.Metadata["sha256"] != file.SHA256 { - return "", &host.Error{Kind: host.ErrorIndeterminate, Operation: "materialize S3 release", Err: fmt.Errorf("immutable release object %q conflicts", file.Path)} + return &host.Error{Kind: host.ErrorIndeterminate, Operation: "materialize S3 release", Err: fmt.Errorf("immutable release object %q conflicts", file.Path)} } - continue + return nil } if err != nil { - return "", infrastructure("materialize S3 release object", err) + return infrastructure("materialize S3 release object", err) } if stored.Size != file.Size || stored.SHA256 != file.SHA256 || stored.Metadata["sha256"] != file.SHA256 { - return "", &host.Error{Kind: host.ErrorInfrastructure, Operation: "verify S3 release object", Err: fmt.Errorf("object %q metadata does not match", file.Path)} + return &host.Error{Kind: host.ErrorInfrastructure, Operation: "verify S3 release object", Err: fmt.Errorf("object %q metadata does not match", file.Path)} } + return nil + }); err != nil { + return "", err } content, err := json.Marshal(descriptor) if err != nil { From cc6fbe692d6c64b8239514f2368b4a245273c909 Mon Sep 17 00:00:00 2001 From: Polina Simonenko Date: Tue, 28 Jul 2026 13:25:15 +0200 Subject: [PATCH 12/22] memoise native package facts within a run --- internal/app/verify.go | 86 ++++++++++++++++++------------- internal/factscache/factscache.go | 86 +++++++++++++++++++++++++++++++ internal/state/cas.go | 36 ++++++++----- internal/state/cas_bench_test.go | 37 +++++++++++++ internal/state/factscache_test.go | 50 ++++++++++++++++++ 5 files changed, 248 insertions(+), 47 deletions(-) create mode 100644 internal/factscache/factscache.go create mode 100644 internal/state/cas_bench_test.go create mode 100644 internal/state/factscache_test.go diff --git a/internal/app/verify.go b/internal/app/verify.go index 3cb863d..ce64a21 100644 --- a/internal/app/verify.go +++ b/internal/app/verify.go @@ -32,6 +32,7 @@ import ( "github.com/shellcell/snailmail/host" "github.com/shellcell/snailmail/internal/buildgraph" "github.com/shellcell/snailmail/internal/domain" + "github.com/shellcell/snailmail/internal/factscache" openpgpsigner "github.com/shellcell/snailmail/signer/openpgp" ) @@ -873,18 +874,23 @@ func verifyPyPIStructure(root string, manifest buildgraph.RepositoryManifest) ([ if len(parts) != 3 || parts[0] != "packages" || parts[1] != file.SHA256 || !pypi.IsDistributionFilename(parts[2]) { return nil, fmt.Errorf("unexpected PyPI repository path %q", file.Path) } - name := filepath.Join(root, filepath.FromSlash(file.Path)) - packageFile, err := os.Open(name) - if err != nil { - return nil, fmt.Errorf("open PyPI package %q: %w", file.Path, err) - } - facts, inspectErr := pypi.Inspect(parts[2], packageFile, file.Size) - closeErr := packageFile.Close() - if inspectErr != nil { - return nil, inspectErr - } - if closeErr != nil { - return nil, fmt.Errorf("close PyPI package %q: %w", file.Path, closeErr) + facts, cached := factscache.Lookup(pypi.FormatID, file.SHA256) + if !cached { + name := filepath.Join(root, filepath.FromSlash(file.Path)) + packageFile, err := os.Open(name) + if err != nil { + return nil, fmt.Errorf("open PyPI package %q: %w", file.Path, err) + } + var inspectErr error + facts, inspectErr = pypi.Inspect(parts[2], packageFile, file.Size) + closeErr := packageFile.Close() + if inspectErr != nil { + return nil, inspectErr + } + if closeErr != nil { + return nil, fmt.Errorf("close PyPI package %q: %w", file.Path, closeErr) + } + factscache.Store(pypi.FormatID, file.SHA256, facts) } blobs = append(blobs, domain.Blob{Filename: parts[2], Size: file.Size, SHA256: file.SHA256, Facts: facts}) } @@ -917,17 +923,22 @@ func verifyDebStructure(root string, manifest buildgraph.RepositoryManifest) ([] return nil, fmt.Errorf("unexpected Debian repository path %q", file.Path) } name := filepath.Join(root, filepath.FromSlash(file.Path)) - packageFile, err := os.Open(name) - if err != nil { - return nil, fmt.Errorf("open Debian package %q: %w", file.Path, err) - } - facts, inspectErr := deb.Inspect(path.Base(file.Path), packageFile, file.Size) - closeErr := packageFile.Close() - if inspectErr != nil { - return nil, inspectErr - } - if closeErr != nil { - return nil, fmt.Errorf("close Debian package %q: %w", file.Path, closeErr) + facts, cached := factscache.Lookup(deb.FormatID, file.SHA256) + if !cached { + packageFile, err := os.Open(name) + if err != nil { + return nil, fmt.Errorf("open Debian package %q: %w", file.Path, err) + } + var inspectErr error + facts, inspectErr = deb.Inspect(path.Base(file.Path), packageFile, file.Size) + closeErr := packageFile.Close() + if inspectErr != nil { + return nil, inspectErr + } + if closeErr != nil { + return nil, fmt.Errorf("close Debian package %q: %w", file.Path, closeErr) + } + factscache.Store(deb.FormatID, file.SHA256, facts) } md5Value, sha1Value, err := legacyChecksums(name) if err != nil { @@ -1019,18 +1030,23 @@ func verifyHelmStructure(root string, manifest buildgraph.RepositoryManifest) ([ if len(parts) != 3 || parts[0] != "charts" || parts[1] != file.SHA256 || !helm.IsChartFilename(parts[2]) { return nil, fmt.Errorf("unexpected Helm repository path %q", file.Path) } - name := filepath.Join(root, filepath.FromSlash(file.Path)) - chartFile, err := os.Open(name) - if err != nil { - return nil, fmt.Errorf("open Helm chart %q: %w", file.Path, err) - } - facts, inspectErr := helm.Inspect(parts[2], chartFile, file.Size) - closeErr := chartFile.Close() - if inspectErr != nil { - return nil, inspectErr - } - if closeErr != nil { - return nil, fmt.Errorf("close Helm chart %q: %w", file.Path, closeErr) + facts, cached := factscache.Lookup(helm.FormatID, file.SHA256) + if !cached { + name := filepath.Join(root, filepath.FromSlash(file.Path)) + chartFile, err := os.Open(name) + if err != nil { + return nil, fmt.Errorf("open Helm chart %q: %w", file.Path, err) + } + var inspectErr error + facts, inspectErr = helm.Inspect(parts[2], chartFile, file.Size) + closeErr := chartFile.Close() + if inspectErr != nil { + return nil, inspectErr + } + if closeErr != nil { + return nil, fmt.Errorf("close Helm chart %q: %w", file.Path, closeErr) + } + factscache.Store(helm.FormatID, file.SHA256, facts) } blobs = append(blobs, domain.Blob{Filename: parts[2], Size: file.Size, SHA256: file.SHA256, Facts: facts}) } diff --git a/internal/factscache/factscache.go b/internal/factscache/factscache.go new file mode 100644 index 0000000..2a41ef5 --- /dev/null +++ b/internal/factscache/factscache.go @@ -0,0 +1,86 @@ +// Package factscache memoises native package facts for the duration of one +// process. +// +// Parsing a package is the single most expensive step in a build: a Debian +// inspection decompresses the entire data archive, up to 256 MiB, only to total +// the installed size. A plan or apply re-derives the same facts several times +// over — once assembling the build input and again verifying the staged tree. +// +// Facts are keyed by format and content digest, which is safe because the +// digest is verified against the lock before anything is cached, and the bytes +// behind a verified digest cannot differ within a run. Nothing is persisted: +// an on-disk cache would have to be trusted across runs, and this is a tool +// whose value comes from re-deriving facts from bytes rather than believing a +// previous answer. +package factscache + +import ( + "sync" + + "github.com/shellcell/snailmail/internal/domain" +) + +// maxEntries bounds the cache so a very large workspace cannot grow it without +// limit. Facts are small, so this is generous; passing it simply stops caching +// rather than evicting, which keeps behaviour independent of insertion order. +const maxEntries = 4096 + +var cache struct { + sync.RWMutex + facts map[string]domain.PackageFacts +} + +func key(format, digest string) string { + return format + "\x00" + digest +} + +// Lookup returns previously derived facts for verified content. +func Lookup(format, digest string) (domain.PackageFacts, bool) { + if digest == "" { + return domain.PackageFacts{}, false + } + cache.RLock() + defer cache.RUnlock() + facts, found := cache.facts[key(format, digest)] + if !found { + return domain.PackageFacts{}, false + } + return clone(facts), true +} + +// Store records facts derived from content whose digest has been verified. +func Store(format, digest string, facts domain.PackageFacts) { + if digest == "" { + return + } + cache.Lock() + defer cache.Unlock() + if cache.facts == nil { + cache.facts = make(map[string]domain.PackageFacts) + } + if len(cache.facts) >= maxEntries { + return + } + cache.facts[key(format, digest)] = clone(facts) +} + +// Reset drops every entry. Tests that rewrite content behind a digest use it. +func Reset() { + cache.Lock() + defer cache.Unlock() + cache.facts = nil +} + +// clone keeps the cached value independent of what a caller does with the +// slice and map it hands back. +func clone(facts domain.PackageFacts) domain.PackageFacts { + facts.Requirements = append([]string(nil), facts.Requirements...) + if facts.Fields != nil { + fields := make(map[string]string, len(facts.Fields)) + for name, value := range facts.Fields { + fields[name] = value + } + facts.Fields = fields + } + return facts +} diff --git a/internal/state/cas.go b/internal/state/cas.go index d4f06ab..894d8c1 100644 --- a/internal/state/cas.go +++ b/internal/state/cas.go @@ -20,6 +20,7 @@ import ( "github.com/shellcell/snailmail/formats/helm" "github.com/shellcell/snailmail/formats/pypi" "github.com/shellcell/snailmail/internal/domain" + "github.com/shellcell/snailmail/internal/factscache" ) func PutArtifact(root, format, sourceName string) (domain.Blob, error) { @@ -377,25 +378,36 @@ func validateLockedBlobOpenContext(ctx context.Context, file *os.File, pathInfo if err != nil { return domain.Blob{}, fmt.Errorf("%w: read blob sha256:%s: %w", blob.ErrUnavailable, locked.SHA256, err) } - if _, err := file.Seek(0, io.SeekStart); err != nil { - return domain.Blob{}, fmt.Errorf("%w: seek blob sha256:%s: %w", blob.ErrUnavailable, locked.SHA256, err) - } - facts, err := inspect(format, locked.Filename, contextReaderAt{ctx: ctx, reader: file}, size) - if err != nil { - if ctx.Err() != nil { - return domain.Blob{}, ctx.Err() - } - return domain.Blob{}, fmt.Errorf("%w: inspect blob sha256:%s: %v", blob.ErrCorrupt, locked.SHA256, err) - } validated := domain.Blob{ Filename: locked.Filename, Size: size, MD5: hex.EncodeToString(md5Hash.Sum(nil)), SHA1: hex.EncodeToString(sha1Hash.Sum(nil)), SHA256: hex.EncodeToString(sha256Hash.Sum(nil)), - Facts: facts, } - if size != info.Size() || validated.Size != locked.Size || validated.SHA256 != locked.SHA256 || (locked.MD5 != "" && validated.MD5 != locked.MD5) || (locked.SHA1 != "" && validated.SHA1 != locked.SHA1) || facts.Architecture != locked.Architecture { + // Establish that these bytes are exactly the locked content before consulting + // any memo, so a cached parse is only ever reused for content this call has + // itself just verified. + if size != info.Size() || validated.Size != locked.Size || validated.SHA256 != locked.SHA256 || + (locked.MD5 != "" && validated.MD5 != locked.MD5) || (locked.SHA1 != "" && validated.SHA1 != locked.SHA1) { + return domain.Blob{}, fmt.Errorf("%w: blob sha256:%s disagrees with its lock", blob.ErrCorrupt, locked.SHA256) + } + facts, cached := factscache.Lookup(format, validated.SHA256) + if !cached { + if _, err := file.Seek(0, io.SeekStart); err != nil { + return domain.Blob{}, fmt.Errorf("%w: seek blob sha256:%s: %w", blob.ErrUnavailable, locked.SHA256, err) + } + facts, err = inspect(format, locked.Filename, contextReaderAt{ctx: ctx, reader: file}, size) + if err != nil { + if ctx.Err() != nil { + return domain.Blob{}, ctx.Err() + } + return domain.Blob{}, fmt.Errorf("%w: inspect blob sha256:%s: %v", blob.ErrCorrupt, locked.SHA256, err) + } + factscache.Store(format, validated.SHA256, facts) + } + validated.Facts = facts + if facts.Architecture != locked.Architecture { return domain.Blob{}, fmt.Errorf("%w: blob sha256:%s disagrees with its lock", blob.ErrCorrupt, locked.SHA256) } if err := ctx.Err(); err != nil { diff --git a/internal/state/cas_bench_test.go b/internal/state/cas_bench_test.go new file mode 100644 index 0000000..f29ce00 --- /dev/null +++ b/internal/state/cas_bench_test.go @@ -0,0 +1,37 @@ +package state + +import ( + "testing" + + "github.com/shellcell/snailmail/internal/factscache" + "github.com/shellcell/snailmail/internal/testutil" +) + +// A single apply validates the same artifact several times: once assembling +// the build input and again for each verification pass over the staged tree. +// +// The synthetic package here is small, so this understates the effect on real +// packages: the memoised step decompresses a Debian data archive in full, up +// to 256 MiB, purely to total the installed size. +func BenchmarkRepeatedBlobValidation(b *testing.B) { + root := b.TempDir() + source, err := testutil.WriteDeb(b.TempDir(), "snail-demo", "1.2.3", "amd64", nil) + if err != nil { + b.Fatal(err) + } + blob, err := PutArtifact(root, "deb", source) + if err != nil { + b.Fatal(err) + } + locked := ToLockedBlob(blob) + b.ReportAllocs() + for b.Loop() { + factscache.Reset() + // Four validations, as one apply performs. + for range 4 { + if _, _, err := LoadBlob(root, "deb", locked); err != nil { + b.Fatal(err) + } + } + } +} diff --git a/internal/state/factscache_test.go b/internal/state/factscache_test.go new file mode 100644 index 0000000..d0b49bd --- /dev/null +++ b/internal/state/factscache_test.go @@ -0,0 +1,50 @@ +package state + +import ( + "os" + "path/filepath" + "testing" + + "github.com/shellcell/snailmail/internal/factscache" + "github.com/shellcell/snailmail/internal/testutil" +) + +// A memo keyed by digest must never let corrupt bytes through: the digest is +// re-derived from the file on every call, and only content that matches its +// lock is allowed to consult previously derived facts. +func TestLockedBlobValidationRejectsCorruptBytesDespiteCachedFacts(t *testing.T) { + factscache.Reset() + t.Cleanup(factscache.Reset) + + root := t.TempDir() + source, err := testutil.WriteWheel(t.TempDir(), "snail-demo", "1.2.3", ">=3.9") + if err != nil { + t.Fatal(err) + } + + blob, err := PutArtifact(root, "pypi", source) + if err != nil { + t.Fatal(err) + } + locked := ToLockedBlob(blob) + + // Prime the memo through a successful validation. + if _, _, err := LoadBlob(root, "pypi", locked); err != nil { + t.Fatal(err) + } + if _, found := factscache.Lookup("pypi", locked.SHA256); !found { + t.Fatal("expected verified facts to be memoised") + } + + // Corrupt the stored object while keeping the lock's digest. + stored := filepath.Join(root, ".snailmail", "cas", "sha256", locked.SHA256[:2], locked.SHA256) + if err := os.Chmod(stored, 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(stored, []byte("not a wheel at all"), 0o600); err != nil { + t.Fatal(err) + } + if _, _, err := LoadBlob(root, "pypi", locked); err == nil { + t.Fatal("corrupt CAS object was accepted because facts were cached") + } +} From 6627162e35fd72f8fa316eb102bb4f92cf395f03 Mon Sep 17 00:00:00 2001 From: Polina Simonenko Date: Tue, 28 Jul 2026 13:30:15 +0200 Subject: [PATCH 13/22] publication lock when Git refuses to proceed --- internal/state/git.go | 59 +++++++++++++++++++++++++++- internal/state/gitlock_test.go | 71 ++++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 2 deletions(-) create mode 100644 internal/state/gitlock_test.go diff --git a/internal/state/git.go b/internal/state/git.go index 65149e9..3f52f77 100644 --- a/internal/state/git.go +++ b/internal/state/git.go @@ -15,6 +15,7 @@ import ( "strings" "sync" "syscall" + "time" ) func AcquireWorkspaceLock(root string) (func(), error) { @@ -488,11 +489,12 @@ func AcquireGitRevisionLock(root, expectedRevision string) (func(), error) { releases[index]() } } - for _, name := range []string{indexPath + ".lock", headPath + ".lock", refPath + ".lock"} { + names := []string{indexPath + ".lock", headPath + ".lock", refPath + ".lock"} + for _, name := range names { release, err := acquireGitLock(name) if err != nil { releaseAll() - return nil, errors.New("Git changed or is busy before publication") + return nil, fmt.Errorf("Git changed or is busy before publication: %w", describeGitLockConflict(name, names, err)) } releases = append(releases, release) } @@ -1022,11 +1024,23 @@ func resolveGitPath(root, name string) (string, error) { return filepath.Clean(resolved), nil } +// gitLockOwnerPrefix marks a lock file this process created, so a lock left +// behind by a crash can be told apart from one Git is genuinely holding. +const gitLockOwnerPrefix = "snailmail publication lock" + func acquireGitLock(name string) (func(), error) { file, err := os.OpenFile(name, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) if err != nil { return nil, err } + // Git only reads a lock file it created itself, so recording who holds this + // one is free and turns an opaque "File exists" into something actionable. + owner := fmt.Sprintf("%s\npid=%d\nsince=%s\n", gitLockOwnerPrefix, os.Getpid(), time.Now().UTC().Format(time.RFC3339)) + if _, err := file.WriteString(owner); err != nil { + _ = file.Close() + _ = os.Remove(name) + return nil, err + } if err := file.Close(); err != nil { _ = os.Remove(name) return nil, err @@ -1034,6 +1048,47 @@ func acquireGitLock(name string) (func(), error) { return func() { _ = os.Remove(name) }, nil } +// describeGitLockConflict explains which lock blocked publication and, when it +// was left behind by an interrupted snailmail run, says so and names every file +// that has to be removed to recover. +func describeGitLockConflict(blocked string, names []string, cause error) error { + if !errors.Is(cause, os.ErrExist) { + return fmt.Errorf("%s: %w", blocked, cause) + } + content, readErr := os.ReadFile(blocked) + if readErr != nil || !strings.HasPrefix(string(content), gitLockOwnerPrefix) { + return fmt.Errorf("%s is held by another Git process", blocked) + } + details := strings.ReplaceAll(strings.TrimSpace(string(content)), "\n", " ") + if owner, found := gitLockOwnerProcess(content); found && processExists(owner) { + return fmt.Errorf("%s is held by a running snailmail process (%s)", blocked, details) + } + return fmt.Errorf("%s was left behind by a snailmail run that did not finish (%s); "+ + "if no snailmail process is publishing this repository, remove %s to recover", + blocked, details, strings.Join(names, ", ")) +} + +func gitLockOwnerProcess(content []byte) (int, bool) { + for _, line := range strings.Split(string(content), "\n") { + if value, found := strings.CutPrefix(line, "pid="); found { + pid, err := strconv.Atoi(strings.TrimSpace(value)) + return pid, err == nil && pid > 0 + } + } + return 0, false +} + +// processExists reports whether a process id is live. A recycled id can make +// this a false positive, which is why the caller only uses it to soften the +// wording rather than to remove anything. +func processExists(pid int) bool { + process, err := os.FindProcess(pid) + if err != nil { + return false + } + return process.Signal(syscall.Signal(0)) == nil +} + func copyGitIndex(indexPath string) (string, error) { source, err := os.Open(indexPath) if err != nil { diff --git a/internal/state/gitlock_test.go b/internal/state/gitlock_test.go new file mode 100644 index 0000000..2e646a4 --- /dev/null +++ b/internal/state/gitlock_test.go @@ -0,0 +1,71 @@ +package state + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestGitLockRecordsItsOwner(t *testing.T) { + name := filepath.Join(t.TempDir(), "index.lock") + release, err := acquireGitLock(name) + if err != nil { + t.Fatal(err) + } + defer release() + content, err := os.ReadFile(name) + if err != nil { + t.Fatal(err) + } + if !strings.HasPrefix(string(content), gitLockOwnerPrefix) { + t.Fatalf("lock file does not record its owner: %q", content) + } + pid, found := gitLockOwnerProcess(content) + if !found || pid != os.Getpid() { + t.Fatalf("recorded pid = %d found=%v, want %d", pid, found, os.Getpid()) + } +} + +// A crash leaves lock files behind that block every later Git operation. The +// failure has to say that snailmail left them and which ones to remove, +// otherwise the operator only sees Git's opaque "File exists". +func TestGitLockConflictExplainsAbandonedLock(t *testing.T) { + directory := t.TempDir() + blocked := filepath.Join(directory, "index.lock") + names := []string{blocked, filepath.Join(directory, "HEAD.lock")} + + abandoned := gitLockOwnerPrefix + "\npid=1\nsince=2026-01-01T00:00:00Z\n" + if err := os.WriteFile(blocked, []byte(abandoned), 0o600); err != nil { + t.Fatal(err) + } + _, err := acquireGitLock(blocked) + if err == nil { + t.Fatal("expected acquiring an existing lock to fail") + } + message := describeGitLockConflict(blocked, names, err).Error() + for _, want := range []string{"snailmail", "remove", blocked, names[1]} { + if !strings.Contains(message, want) { + t.Fatalf("message %q does not mention %q", message, want) + } + } +} + +func TestGitLockConflictDoesNotClaimForeignLocks(t *testing.T) { + directory := t.TempDir() + blocked := filepath.Join(directory, "index.lock") + if err := os.WriteFile(blocked, []byte("some other git process"), 0o600); err != nil { + t.Fatal(err) + } + _, err := acquireGitLock(blocked) + if err == nil { + t.Fatal("expected acquiring an existing lock to fail") + } + message := describeGitLockConflict(blocked, []string{blocked}, err).Error() + if strings.Contains(message, "remove") { + t.Fatalf("a lock snailmail does not own must not be recommended for removal: %q", message) + } + if !strings.Contains(message, "another Git process") { + t.Fatalf("message %q does not attribute the lock to Git", message) + } +} From bb1f485d6bb96211b2d8f8d94d9696d68803c6c0 Mon Sep 17 00:00:00 2001 From: Polina Simonenko Date: Tue, 28 Jul 2026 13:33:58 +0200 Subject: [PATCH 14/22] reuse the verified stage manifest and link repository snapshots --- engine/engine.go | 6 ++++++ engine/workspace.go | 38 ++++++++++++++++++++------------------ internal/app/verify.go | 21 ++++++++++++++++++++- 3 files changed, 46 insertions(+), 19 deletions(-) diff --git a/engine/engine.go b/engine/engine.go index 6f2010f..1e42171 100644 --- a/engine/engine.go +++ b/engine/engine.go @@ -79,6 +79,9 @@ type VerifyResult struct { TreeSHA256 string FileCount int InstalledCases int + // Manifest is the verified file list, returned so a caller that has just + // verified a tree does not have to re-verify it to enumerate its contents. + Manifest buildgraph.RepositoryManifest } type RepositoryInfo struct { @@ -273,6 +276,7 @@ func VerifyPyPI(ctx context.Context, request VerifyPyPIRequest) (VerifyResult, e TreeSHA256: manifest.TreeSHA256, FileCount: len(manifest.Files), InstalledCases: installed, + Manifest: manifest, }, nil } @@ -304,6 +308,7 @@ func VerifyDeb(ctx context.Context, request VerifyDebRequest) (VerifyResult, err TreeSHA256: manifest.TreeSHA256, FileCount: len(manifest.Files), InstalledCases: installed, + Manifest: manifest, }, nil } @@ -331,6 +336,7 @@ func VerifyHelm(ctx context.Context, request VerifyHelmRequest) (VerifyResult, e TreeSHA256: manifest.TreeSHA256, FileCount: len(manifest.Files), InstalledCases: verified, + Manifest: manifest, }, nil } diff --git a/engine/workspace.go b/engine/workspace.go index 42a8d48..0e34f8c 100644 --- a/engine/workspace.go +++ b/engine/workspace.go @@ -705,6 +705,7 @@ func ApplyWorkspace(ctx context.Context, request ApplyWorkspaceRequest) (ApplyWo hostStage host.StagedPublication stageRoot string stage string + stagedManifest buildgraph.RepositoryManifest current bool deployment state.DeploymentRecord signingState deploymentSigningState @@ -885,10 +886,11 @@ func ApplyWorkspace(ctx context.Context, request ApplyWorkspaceRequest) (ApplyWo if buildErr == nil && !signingContentsEqual(staged.Signing, planned.Signing) { buildErr = errors.New("stale plan: signing recipe changed") } + var stagedManifest buildgraph.RepositoryManifest if buildErr == nil { structuralRequest := request structuralRequest.StructuralOnly = true - buildErr = verifyStaged(ctx, repository.Format, stageOutput, structuralRequest) + stagedManifest, buildErr = verifyStaged(ctx, repository.Format, stageOutput, structuralRequest) } if buildErr != nil { _ = os.RemoveAll(stage) @@ -897,7 +899,7 @@ func ApplyWorkspace(ctx context.Context, request ApplyWorkspaceRequest) (ApplyWo } return ApplyWorkspaceResult{}, buildErr } - item.stageRoot, item.stage = stage, stageOutput + item.stageRoot, item.stage, item.stagedManifest = stage, stageOutput, stagedManifest prepared = append(prepared, item) } defer func() { @@ -947,7 +949,7 @@ func ApplyWorkspace(ctx context.Context, request ApplyWorkspaceRequest) (ApplyWo if item.current { continue } - files, err := stagedHostFiles(item.stage) + files, err := stagedHostFiles(item.stage, item.stagedManifest) if err != nil { return ApplyWorkspaceResult{}, err } @@ -966,7 +968,7 @@ func ApplyWorkspace(ctx context.Context, request ApplyWorkspaceRequest) (ApplyWo continue } if item.hostRepository.Type == "local" { - if err := verifyStaged(ctx, item.repository.Format, item.stage, request); err != nil { + if _, err := verifyStaged(ctx, item.repository.Format, item.stage, request); err != nil { return ApplyWorkspaceResult{}, err } continue @@ -1446,7 +1448,8 @@ func verifyCanonicalClient(ctx context.Context, root string, repository state.Re if err != nil { return err } - return verifyStaged(ctx, repository.Format, output, request) + _, err = verifyStaged(ctx, repository.Format, output, request) + return err } if repository.Format != "pypi" { return fmt.Errorf("canonical client verification is not implemented for format %q", repository.Format) @@ -1664,19 +1667,19 @@ func planAcquisitionsForVersions(versions []state.PackageVersion) []state.PlanAc return acquisitions } -func verifyStaged(ctx context.Context, format, repository string, request ApplyWorkspaceRequest) error { +func verifyStaged(ctx context.Context, format, repository string, request ApplyWorkspaceRequest) (buildgraph.RepositoryManifest, error) { switch format { case "pypi": - _, err := VerifyPyPI(ctx, VerifyPyPIRequest{Repository: repository, Python: request.Python, StructuralOnly: request.StructuralOnly}) - return err + result, err := VerifyPyPI(ctx, VerifyPyPIRequest{Repository: repository, Python: request.Python, StructuralOnly: request.StructuralOnly}) + return result.Manifest, err case "deb": - _, err := VerifyDeb(ctx, VerifyDebRequest{Repository: repository, Runner: request.Runner, Image: request.DebianImage, MaxWorkspaceBytes: request.MaxWorkspaceBytes, StructuralOnly: request.StructuralOnly}) - return err + result, err := VerifyDeb(ctx, VerifyDebRequest{Repository: repository, Runner: request.Runner, Image: request.DebianImage, MaxWorkspaceBytes: request.MaxWorkspaceBytes, StructuralOnly: request.StructuralOnly}) + return result.Manifest, err case "helm": - _, err := VerifyHelm(ctx, VerifyHelmRequest{Repository: repository, Runner: request.Runner, Image: request.HelmImage, StructuralOnly: request.StructuralOnly}) - return err + result, err := VerifyHelm(ctx, VerifyHelmRequest{Repository: repository, Runner: request.Runner, Image: request.HelmImage, StructuralOnly: request.StructuralOnly}) + return result.Manifest, err default: - return fmt.Errorf("unsupported repository format %q", format) + return buildgraph.RepositoryManifest{}, fmt.Errorf("unsupported repository format %q", format) } } @@ -1811,11 +1814,10 @@ func repositoryInstallDocDigest(root, name string, repository state.Repository) return state.HashFile(filename) } -func stagedHostFiles(directory string) ([]host.File, error) { - manifest, err := app.VerifyRepository(directory) - if err != nil { - return nil, err - } +// stagedHostFiles enumerates a stage that this apply has already verified in +// full. The host verifies the directory again when it stages it, so the file +// list is a convenience here rather than the integrity boundary. +func stagedHostFiles(directory string, manifest buildgraph.RepositoryManifest) ([]host.File, error) { files := make([]host.File, 0, len(manifest.Files)+1) for _, file := range manifest.Files { files = append(files, host.File{Path: file.Path, Size: file.Size, SHA256: file.SHA256}) diff --git a/internal/app/verify.go b/internal/app/verify.go index ce64a21..24544a0 100644 --- a/internal/app/verify.go +++ b/internal/app/verify.go @@ -1128,7 +1128,7 @@ func snapshotRepository(ctx context.Context, source string, manifest buildgraph. if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { return "", err } - if err := copyFile(name, target, file.Size); err != nil { + if err := linkOrCopyFile(name, target, file.Size); err != nil { return "", err } } @@ -1136,6 +1136,25 @@ func snapshotRepository(ctx context.Context, source string, manifest buildgraph. return snapshot, nil } +// linkOrCopyFile prefers a hard link, so snapshotting a repository that can +// reach the 4 GiB verification limit does not duplicate every byte. The link +// shares the inode with an immutable release file, and the snapshot is only +// ever read, so the two stay identical by construction. A link across +// filesystems, or onto one that does not support them, falls back to copying. +func linkOrCopyFile(sourceName, targetName string, expectedSize int64) error { + if err := os.Link(sourceName, targetName); err == nil { + info, statErr := os.Lstat(targetName) + if statErr != nil { + return statErr + } + if !info.Mode().IsRegular() || info.Size() != expectedSize { + return fmt.Errorf("repository file %q changed while snapshotting", sourceName) + } + return nil + } + return copyFile(sourceName, targetName, expectedSize) +} + func copyFile(sourceName, targetName string, expectedSize int64) error { source, err := os.Open(sourceName) if err != nil { From e002c7e91abf2732d8aebc94f9d94fb5a10346bc Mon Sep 17 00:00:00 2001 From: Polina Simonenko Date: Tue, 28 Jul 2026 13:35:10 +0200 Subject: [PATCH 15/22] report the lowest failing S3 request deterministically --- adapters/host/s3/parallel.go | 35 ++++++++++++++--------------------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/adapters/host/s3/parallel.go b/adapters/host/s3/parallel.go index 4b7a583..58e5383 100644 --- a/adapters/host/s3/parallel.go +++ b/adapters/host/s3/parallel.go @@ -4,6 +4,7 @@ import ( "context" "runtime" "sync" + "sync/atomic" ) // defaultObjectConcurrency bounds in-flight object requests. Publication is @@ -15,6 +16,12 @@ var defaultObjectConcurrency = min(16, max(4, runtime.GOMAXPROCS(0)*4)) // forEachObject runs work over every index in [0, count) with bounded // concurrency and returns the error belonging to the lowest index, so that a // failure is reported identically no matter which request lost the race. +// +// The first failure stops further indices being dispatched, but requests +// already in flight are allowed to finish against the caller's context rather +// than being cancelled. Cancelling them would replace a genuine failure at a +// lower index with a self-inflicted "context canceled", which is both +// misleading and non-deterministic. func forEachObject(ctx context.Context, count int, work func(ctx context.Context, index int) error) error { if count == 0 { return nil @@ -23,11 +30,8 @@ func forEachObject(ctx context.Context, count int, work func(ctx context.Context return work(ctx, 0) } workers := min(defaultObjectConcurrency, count) - // A cancelled context stops the remaining work as soon as one request fails. - groupCtx, cancel := context.WithCancel(ctx) - defer cancel() - errs := make([]error, count) + var failed atomic.Bool next := make(chan int) var waitGroup sync.WaitGroup waitGroup.Add(workers) @@ -35,37 +39,26 @@ func forEachObject(ctx context.Context, count int, work func(ctx context.Context go func() { defer waitGroup.Done() for index := range next { - if groupCtx.Err() != nil { - return - } - if err := work(groupCtx, index); err != nil { + if err := work(ctx, index); err != nil { errs[index] = err - cancel() - return + failed.Store(true) } } }() } for index := range count { - select { - case next <- index: - case <-groupCtx.Done(): - close(next) - waitGroup.Wait() - return firstError(errs, ctx) + if failed.Load() || ctx.Err() != nil { + break } + next <- index } close(next) waitGroup.Wait() - return firstError(errs, ctx) -} - -func firstError(errs []error, ctx context.Context) error { for _, err := range errs { if err != nil { return err } } - // No worker recorded a failure, so any stop came from the caller's context. + // No request recorded a failure, so any stop came from the caller. return ctx.Err() } From aa353a730aa1cca216690177335ba873db11c23f Mon Sep 17 00:00:00 2001 From: Polina Simonenko Date: Tue, 28 Jul 2026 13:37:35 +0200 Subject: [PATCH 16/22] confirm HEAD rather than revalidating the workspace per repository --- engine/workspace.go | 10 ++++++++-- internal/state/git.go | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/engine/workspace.go b/engine/workspace.go index 0e34f8c..fc547b6 100644 --- a/engine/workspace.go +++ b/engine/workspace.go @@ -1116,7 +1116,10 @@ func ApplyWorkspace(ctx context.Context, request ApplyWorkspaceRequest) (ApplyWo } continue } - if err := state.AssertGitRevision(root, applyGitRevision); err != nil { + // The publication locks are held for this whole loop, so Git cannot move + // the branch under it; confirming HEAD is enough and keeps the cost of a + // publication independent of how many repositories it covers. + if err := state.AssertGitHeadRevision(root, applyGitRevision); err != nil { return result, err } if err := authorize(item); err != nil { @@ -1176,7 +1179,10 @@ func ApplyWorkspace(ctx context.Context, request ApplyWorkspaceRequest) (ApplyWo return result, fmt.Errorf("canonical client probe failed: %w", probeErr) } } - if err := state.AssertGitRevision(root, applyGitRevision); err != nil { + // The publication locks are held for this whole loop, so Git cannot move + // the branch under it; confirming HEAD is enough and keeps the cost of a + // publication independent of how many repositories it covers. + if err := state.AssertGitHeadRevision(root, applyGitRevision); err != nil { return result, err } deployments = append(deployments, deploymentRecordFor(item.planned, item.deployment, item.observed, item.signingState, committed.Revision.NativeRevision, plan.PlanID, plan.Payload.CreatedAt, request.currentTime())) diff --git a/internal/state/git.go b/internal/state/git.go index 3f52f77..0cbe645 100644 --- a/internal/state/git.go +++ b/internal/state/git.go @@ -461,6 +461,24 @@ func AssertGitRevisionContext(ctx context.Context, root, expected string) error return nil } +// AssertGitHeadRevision checks only that HEAD still names the expected commit. +// +// It is for callers holding the locks AcquireGitRevisionLock takes, which +// already prevent Git from moving the branch, updating the index or checking +// anything out. Under those locks the full workspace validation cannot find +// anything new, and repeating it once per repository made a publication cost +// grow with the square of the repository count. +func AssertGitHeadRevision(root, expected string) error { + current, err := gitOutput(root, "rev-parse", "HEAD") + if err != nil { + return fmt.Errorf("read Git revision during publication: %w", err) + } + if current != expected { + return errors.New("Git revision changed during operation") + } + return nil +} + // AcquireGitRevisionLock blocks normal branch, index, and checkout updates while // a verified target is switched to the tree authorized by expectedRevision. func AcquireGitRevisionLock(root, expectedRevision string) (func(), error) { From 3401e2eac650fdb745dc8a87cbbc159007490598 Mon Sep 17 00:00:00 2001 From: Polina Simonenko Date: Tue, 28 Jul 2026 13:46:03 +0200 Subject: [PATCH 17/22] split apply into preparation, publication and compensation --- engine/workspace.go | 659 +++++++++++++++++++++++++------------------- 1 file changed, 369 insertions(+), 290 deletions(-) diff --git a/engine/workspace.go b/engine/workspace.go index fc547b6..d449c16 100644 --- a/engine/workspace.go +++ b/engine/workspace.go @@ -617,6 +617,41 @@ func PlanWorkspace(ctx context.Context, request PlanWorkspaceRequest) (PlanWorks return PlanWorkspaceResult{PlanID: plan.PlanID, Output: output, Changes: changes, Acquisitions: plannedAcquisitions}, nil } +// applyRepository is one repository's state as apply carries it from plan +// revalidation through staging to publication. +type applyRepository struct { + planned state.PlanRepository + repository state.Repository + lock state.RepositoryLock + host host.Host + hostRepository host.Repository + observed host.PublishedRevision + hostStage host.StagedPublication + stageRoot string + stage string + stagedManifest buildgraph.RepositoryManifest + current bool + deployment state.DeploymentRecord + signingState deploymentSigningState +} + +// restoreFailedPublication compensates a failed verification by asking the host +// to put back the revision this change displaced. Restoring is itself a +// publication effect, so it is gated exactly like the change that failed. +func restoreFailedPublication(ctx context.Context, item applyRepository, reference host.RestoreRef, expected host.ExpectedRevision, authorize func(applyRepository) error) error { + if err := authorize(item); err != nil { + return fmt.Errorf("restore gate failed: %w", err) + } + // The restore must still run when the caller's context is already done, + // otherwise an interrupt would leave the failed tree published. + restoreCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 2*time.Minute) + defer cancel() + if _, err := item.host.Restore(restoreCtx, item.hostRepository, reference, expected); err != nil { + return fmt.Errorf("restore failed: %w", err) + } + return nil +} + func ApplyWorkspace(ctx context.Context, request ApplyWorkspaceRequest) (ApplyWorkspaceResult, error) { root, err := workspaceRoot(request.Root) if err != nil { @@ -695,211 +730,29 @@ func ApplyWorkspace(ctx context.Context, request ApplyWorkspaceRequest) (ApplyWo if err != nil { return ApplyWorkspaceResult{}, err } - type applyRepository struct { - planned state.PlanRepository - repository state.Repository - lock state.RepositoryLock - host host.Host - hostRepository host.Repository - observed host.PublishedRevision - hostStage host.StagedPublication - stageRoot string - stage string - stagedManifest buildgraph.RepositoryManifest - current bool - deployment state.DeploymentRecord - signingState deploymentSigningState - } var prepared []applyRepository hosts := request.Hosts if hosts == nil { hosts = localHostResolver{} } seenRepositories := make(map[string]bool) + preparation := &applyPreparation{ + ctx: ctx, root: root, request: request, plan: plan, manifest: manifest, hosts: hosts, + blobStore: blobStore, now: now, expiresAt: expiresAt, generatedAt: generatedAt, + signatureTime: signatureTime, ledgerCommitted: ledgerCommitted, + } for _, planned := range plan.Payload.Repositories { if seenRepositories[planned.Name] { return ApplyWorkspaceResult{}, fmt.Errorf("plan contains duplicate repository %q", planned.Name) } seenRepositories[planned.Name] = true - repository, exists := manifest.Repositories[planned.Name] - if !exists || repository.Format != planned.Format || repository.Gate != planned.Gate || !reflect.DeepEqual(repository.ApprovalKeys, planned.ApprovalKeys) || len(planned.Signing) > 1 || (len(planned.Signing) == 0) != (len(repository.SigningKeys) == 0) { - return ApplyWorkspaceResult{}, fmt.Errorf("stale plan: repository %q configuration changed", planned.Name) - } - activeSigningKey, _, _, _, signingStateErr := repositorySigningState(repository) - if signingStateErr != nil { - return ApplyWorkspaceResult{}, signingStateErr - } - for index, signing := range planned.Signing { - if index != 0 || signing.KeyName != activeSigningKey { - return ApplyWorkspaceResult{}, fmt.Errorf("stale plan: repository %q signing key changed", planned.Name) - } - key, exists := manifest.Keys[signing.KeyName] - if !exists { - return ApplyWorkspaceResult{}, fmt.Errorf("stale plan: repository %q signing key is missing", planned.Name) - } - keyExpiresAt, err := time.Parse(time.RFC3339, key.ExpiresAt) - if err != nil || expiresAt.After(keyExpiresAt) { - return ApplyWorkspaceResult{}, fmt.Errorf("repository %q plan expires after its signing key", planned.Name) - } - if err := validateSigningRecipeMetadata(signing, repository.Suite); err != nil { - return ApplyWorkspaceResult{}, fmt.Errorf("plan repository %q: %w", planned.Name, err) - } - } - hostIdentity, err := repositoryHostIdentity(repository) - if err != nil || hostIdentity != planned.HostIdentitySHA256 || repository.Host != planned.Host || repository.Visibility != planned.Visibility { - return ApplyWorkspaceResult{}, fmt.Errorf("stale plan: repository %q host changed", planned.Name) - } - hostRepository := toHostRepository(root, manifest.Workspace.ID, hostIdentity, planned.Name, repository) - if hostRepository.CanonicalEndpoint != planned.CanonicalEndpoint { - return ApplyWorkspaceResult{}, fmt.Errorf("stale plan: repository %q canonical endpoint changed", planned.Name) - } - installDocDigest, err := repositoryInstallDocDigest(root, planned.Name, repository) - if err != nil || installDocDigest != planned.InstallDocSHA256 { - return ApplyWorkspaceResult{}, fmt.Errorf("stale plan: repository %q install document changed", planned.Name) - } - selectedHost, err := hosts.Resolve(ctx, hostRepository) - if err != nil { - return ApplyWorkspaceResult{}, err - } - capabilities, err := selectedHost.Capabilities(ctx, hostRepository) - if err != nil { - return ApplyWorkspaceResult{}, err - } - if capabilities.FaithfulPreview != planned.FaithfulPreview || capabilities.ConditionalCommit != planned.ConditionalCommit || capabilities.ConditionalRestore != planned.ConditionalRestore || - capabilities.PrivateRead != planned.PrivateRead || capabilities.CredentialBrokerIdentity != planned.CredentialBrokerIdentity { - return ApplyWorkspaceResult{}, fmt.Errorf("stale plan: repository %q host capabilities changed", planned.Name) - } - lockPath, err := state.WorkspacePath(root, repository.Lock) - if err != nil { - return ApplyWorkspaceResult{}, err - } - lockDigest, err := state.HashFile(lockPath) - if err != nil || lockDigest != planned.LockSHA256 { - return ApplyWorkspaceResult{}, fmt.Errorf("stale plan: repository %q lock changed", planned.Name) - } - lock, err := state.LoadLock(root, repository) - if err != nil { - return ApplyWorkspaceResult{}, err - } - if err := state.ValidateLock(lock, planned.Name, repository.Format); err != nil { - return ApplyWorkspaceResult{}, err - } - ledger, err := state.LoadLedgerHistory(root, planned.Name) - if err != nil { - return ApplyWorkspaceResult{}, err - } - if err := state.ValidatePublicationHistory(planned.Name, ledger); err != nil { - return ApplyWorkspaceResult{}, err - } - if err := state.ValidatePublishedBindings(lock, ledger); err != nil { - return ApplyWorkspaceResult{}, err - } - missingBindings := missingPublicationBindings(lock, repository, ledger) - expectedBindings := []state.PlanPublicationBinding(nil) - if planned.PublicationRecords { - expectedBindings = publicationBindingsForVersions(visiblePackageVersions(lock, repository)) - } - expectedAcquisitions := planAcquisitionsForVersions(visiblePackageVersions(lock, repository)) - if !reflect.DeepEqual(planned.Acquisitions, expectedAcquisitions) { - return ApplyWorkspaceResult{}, fmt.Errorf("plan repository %q has inconsistent adopted acquisitions", planned.Name) - } - if planned.PublicationRecords != (len(planned.PublicationBindings) != 0) || - !reflect.DeepEqual(planned.PublicationBindings, expectedBindings) || - (!ledgerCommitted && planned.PublicationRecords != (len(missingBindings) != 0)) || - (ledgerCommitted && !planned.PublicationRecords && len(missingBindings) != 0) { - return ApplyWorkspaceResult{}, fmt.Errorf("plan repository %q has inconsistent publication-record effects", planned.Name) - } - deployment, err := state.LoadDeployment(root, planned.Name) - if err != nil { - return ApplyWorkspaceResult{}, err - } - desiredSigningState, err := repositoryDeploymentSigningState(repository, manifest.Keys) - if err != nil { - return ApplyWorkspaceResult{}, err - } - observed, err := selectedHost.Observe(ctx, hostRepository) - if err != nil { - return ApplyWorkspaceResult{}, err - } - plannedObserved := publishedFromPlanObservation(planned) - trustNotBefore := time.Time{} - if repository.SigningRotation != nil || deployment.SigningRotationPhase != "" { - trustNotBefore, err = state.AuthoritativeDeploymentTrustSince(root, planned.Name, deployment) - if err != nil { - return ApplyWorkspaceResult{}, err - } - } - if err := validateRepositorySigningTransition(repository, manifest.Keys, deployment, plannedObserved, now, trustNotBefore); err != nil { - return ApplyWorkspaceResult{}, fmt.Errorf("repository %q: %w", planned.Name, err) - } - matchesObserved := revisionMatchesPlanObservation(observed, planned) - managedRemote := repository.Host.Type == "s3" || repository.Host.Type == "github-pages" - matchesApplied := planned.Action != "noop" && observed.TreeSHA256 == planned.DesiredTreeSHA256 && - (!managedRemote || (observed.PlanID == plan.PlanID && observed.ChangeID == planned.ChangeID && observed.ManifestSHA256 == planned.DesiredManifestSHA256)) - deploymentApplied := deployment.PlanID == plan.PlanID && deployment.ChangeID == planned.ChangeID && deployment.TreeSHA256 == planned.DesiredTreeSHA256 && deployment.ManifestSHA256 == planned.DesiredManifestSHA256 && deployment.NativeRevision == observed.NativeRevision && deploymentSigningMatches(deployment, desiredSigningState) - deploymentCurrent := deploymentMatchesDesired(deployment, observed, planned.DesiredTreeSHA256, planned.DesiredManifestSHA256, desiredSigningState) - if !reflect.DeepEqual(deployment, planned.ObservedDeployment) && !deploymentApplied { - return ApplyWorkspaceResult{}, fmt.Errorf("stale plan: repository %q deployment receipt changed", planned.Name) - } - if !matchesObserved && !matchesApplied { - if observed.TreeSHA256 == planned.ObservedTreeSHA256 { - return ApplyWorkspaceResult{}, fmt.Errorf("stale plan: repository %q native revision changed", planned.Name) - } - if observed.TreeSHA256 == planned.DesiredTreeSHA256 { - return ApplyWorkspaceResult{}, fmt.Errorf("stale plan: repository %q desired tree was published by another change", planned.Name) - } - return ApplyWorkspaceResult{}, fmt.Errorf("stale plan: repository %q target changed", planned.Name) - } - expectedAction := "noop" - if planned.ObservedTreeSHA256 != planned.DesiredTreeSHA256 || (managedRemote && planned.ObservedManifestSHA256 != planned.DesiredManifestSHA256) || !publicationBindingsComplete(lock, repository, ledger) || !deploymentMatchesDesired(planned.ObservedDeployment, plannedObserved, planned.DesiredTreeSHA256, planned.DesiredManifestSHA256, desiredSigningState) { - expectedAction = "update" - if planned.ObservedRevision == "" { - expectedAction = "create" - } - } - if planned.Action != expectedAction || planned.ChangeID != planned.Name+":"+planned.DesiredTreeSHA256[:12] { - return ApplyWorkspaceResult{}, fmt.Errorf("plan repository %q has inconsistent action metadata", planned.Name) - } - receiptRecovery := matchesApplied && reflect.DeepEqual(deployment, planned.ObservedDeployment) - current := observed.TreeSHA256 == planned.DesiredTreeSHA256 && (!managedRemote || observed.ManifestSHA256 == planned.DesiredManifestSHA256) && (deploymentApplied || (planned.Action == "noop" && deploymentCurrent) || receiptRecovery) - item := applyRepository{ - planned: planned, repository: repository, lock: lock, host: selectedHost, - hostRepository: hostRepository, observed: observed, current: current, deployment: deployment, signingState: desiredSigningState, - } - if item.current && (request.StructuralOnly || planned.Action == "noop") && !(planned.Action != "noop" && len(planned.Signing) != 0) { - prepared = append(prepared, item) - continue - } - staging, err := stagingRoot(root) - if err != nil { - return ApplyWorkspaceResult{}, err - } - stage, err := os.MkdirTemp(staging, ".snailmail-apply-*") + item, err := preparation.prepareRepository(planned) if err != nil { - return ApplyWorkspaceResult{}, err - } - stageOutput := filepath.Join(stage, "repository") - staged, buildErr := buildLockedRepository(ctx, root, planned.Name, repository, lock, generatedAt, signatureTime, stageOutput, blobStore, manifest.Keys, planned.Signing, nil) - if buildErr == nil && (staged.TreeSHA256 != planned.DesiredTreeSHA256 || staged.ManifestSHA256 != planned.DesiredManifestSHA256) { - buildErr = errors.New("stale plan: rebuilt tree digest changed") - } - if buildErr == nil && !signingContentsEqual(staged.Signing, planned.Signing) { - buildErr = errors.New("stale plan: signing recipe changed") - } - var stagedManifest buildgraph.RepositoryManifest - if buildErr == nil { - structuralRequest := request - structuralRequest.StructuralOnly = true - stagedManifest, buildErr = verifyStaged(ctx, repository.Format, stageOutput, structuralRequest) - } - if buildErr != nil { - _ = os.RemoveAll(stage) for _, previous := range prepared { _ = os.RemoveAll(previous.stageRoot) } - return ApplyWorkspaceResult{}, buildErr + return ApplyWorkspaceResult{}, err } - item.stageRoot, item.stage, item.stagedManifest = stage, stageOutput, stagedManifest prepared = append(prepared, item) } defer func() { @@ -1083,113 +936,22 @@ func ApplyWorkspace(ctx context.Context, request ApplyWorkspaceRequest) (ApplyWo } result := ApplyWorkspaceResult{PlanID: plan.PlanID} deployments := make([]state.DeploymentRecord, 0, len(prepared)) + execution := &applyExecution{ + ctx: ctx, root: root, request: request, plan: plan, + applyGitRevision: applyGitRevision, authorize: authorize, result: result, + } for _, item := range prepared { + var err error if item.current { - result.Current++ - if !request.StructuralOnly && item.planned.Action != "noop" { - access, accessErr := item.host.ReadAccess(ctx, item.hostRepository, item.observed) - if accessErr != nil { - return result, accessErr - } - if err := verifyCanonicalClient(ctx, root, item.repository, item.stage, access, request); err != nil { - if item.observed.RestoreID != "" { - if gateErr := authorize(item); gateErr != nil { - return result, gateErr - } - restoreCtx, cancelRestore := context.WithTimeout(context.WithoutCancel(ctx), 2*time.Minute) - _, restoreErr := item.host.Restore(restoreCtx, item.hostRepository, host.RestoreRef{ - ID: item.observed.RestoreID, PlanID: item.observed.PlanID, - ChangeID: item.observed.ChangeID, FailedTree: item.observed.TreeSHA256, - DescriptorSHA256: item.observed.RestoreSHA256, RootSHA256: item.observed.RestoreRootSHA256, - }, expectedRevisionFromPublished(item.observed)) - cancelRestore() - if restoreErr != nil { - return result, fmt.Errorf("canonical retry probe failed: %v; restore failed: %w", err, restoreErr) - } - result.Current-- - } - return result, err - } - } - if item.planned.Action != "noop" { - deployments = append(deployments, deploymentRecordFor(item.planned, item.deployment, item.observed, item.signingState, item.observed.NativeRevision, plan.PlanID, plan.Payload.CreatedAt, request.currentTime())) - } - continue - } - // The publication locks are held for this whole loop, so Git cannot move - // the branch under it; confirming HEAD is enough and keeps the cost of a - // publication independent of how many repositories it covers. - if err := state.AssertGitHeadRevision(root, applyGitRevision); err != nil { - return result, err - } - if err := authorize(item); err != nil { - return result, err + err = execution.verifyCurrent(item) + } else { + err = execution.publish(item) } - committed, err := item.host.Commit(ctx, item.hostRepository, item.hostStage, expectedRevisionFromPlan(item.planned)) if err != nil { - return result, err - } - if committed.Access.Credential != nil { - // The deferred destroy covers the error paths below, which return - // immediately. Successful iterations destroy their own credential at - // the end so that a multi-repository apply does not accumulate live - // short-lived credentials until the whole apply finishes. - defer committed.Access.Credential.Destroy() - } - result.Applied++ - canonical, observeErr := item.host.Observe(ctx, item.hostRepository) - if observeErr != nil || canonical.TreeSHA256 != item.planned.DesiredTreeSHA256 || canonical.NativeRevision != committed.Revision.NativeRevision || - (item.hostRepository.Type == "s3" && canonical != committed.Revision) { - probeErr := observeErr - if probeErr == nil { - probeErr = errors.New("canonical host observation does not match committed tree") - } - if committed.RestoreRef != nil { - if gateErr := authorize(item); gateErr != nil { - return result, fmt.Errorf("canonical probe failed: %v; restore gate failed: %w", probeErr, gateErr) - } - restoreCtx, cancelRestore := context.WithTimeout(context.WithoutCancel(ctx), 2*time.Minute) - _, restoreErr := item.host.Restore(restoreCtx, item.hostRepository, *committed.RestoreRef, expectedRevisionFromPublished(committed.Revision)) - cancelRestore() - if restoreErr != nil { - return result, fmt.Errorf("canonical probe failed: %v; restore failed: %w", probeErr, restoreErr) - } - result.Applied-- - } - return result, fmt.Errorf("canonical probe failed: %w", probeErr) - } - if !request.StructuralOnly { - access := committed.Access - if access.Endpoint == "" { - access.Endpoint = committed.CanonicalEndpoint - } - if probeErr := verifyCanonicalClient(ctx, root, item.repository, item.stage, access, request); probeErr != nil { - if committed.RestoreRef != nil { - if gateErr := authorize(item); gateErr != nil { - return result, fmt.Errorf("canonical client probe failed: %v; restore gate failed: %w", probeErr, gateErr) - } - restoreCtx, cancelRestore := context.WithTimeout(context.WithoutCancel(ctx), 2*time.Minute) - _, restoreErr := item.host.Restore(restoreCtx, item.hostRepository, *committed.RestoreRef, expectedRevisionFromPublished(committed.Revision)) - cancelRestore() - if restoreErr != nil { - return result, fmt.Errorf("canonical client probe failed: %v; restore failed: %w", probeErr, restoreErr) - } - result.Applied-- - } - return result, fmt.Errorf("canonical client probe failed: %w", probeErr) - } - } - // The publication locks are held for this whole loop, so Git cannot move - // the branch under it; confirming HEAD is enough and keeps the cost of a - // publication independent of how many repositories it covers. - if err := state.AssertGitHeadRevision(root, applyGitRevision); err != nil { - return result, err - } - deployments = append(deployments, deploymentRecordFor(item.planned, item.deployment, item.observed, item.signingState, committed.Revision.NativeRevision, plan.PlanID, plan.Payload.CreatedAt, request.currentTime())) - if committed.Access.Credential != nil { - committed.Access.Credential.Destroy() + return execution.result, err } } + result, deployments = execution.result, execution.deployments if unlockGit != nil { unlockGit() unlockGit = nil @@ -2115,3 +1877,320 @@ func PlanSummary(plan state.Plan) string { } return strings.Join(lines, "\n") } + +// applyExecution carries the mutable outcome of a publication pass so that each +// repository can be handled by its own function rather than by another few +// hundred lines inside ApplyWorkspace. +type applyExecution struct { + ctx context.Context + root string + request ApplyWorkspaceRequest + plan state.Plan + applyGitRevision string + authorize func(applyRepository) error + + result ApplyWorkspaceResult + deployments []state.DeploymentRecord +} + +// verifyCurrent handles a repository the host already serves at the desired +// tree. Nothing is published, but a client probe still has to pass before the +// deployment receipt can claim the tree is being served. +func (execution *applyExecution) verifyCurrent(item applyRepository) error { + ctx, request := execution.ctx, execution.request + execution.result.Current++ + if !request.StructuralOnly && item.planned.Action != "noop" { + access, accessErr := item.host.ReadAccess(ctx, item.hostRepository, item.observed) + if accessErr != nil { + return accessErr + } + if err := verifyCanonicalClient(ctx, execution.root, item.repository, item.stage, access, request); err != nil { + if item.observed.RestoreID != "" { + reference := host.RestoreRef{ + ID: item.observed.RestoreID, PlanID: item.observed.PlanID, + ChangeID: item.observed.ChangeID, FailedTree: item.observed.TreeSHA256, + DescriptorSHA256: item.observed.RestoreSHA256, RootSHA256: item.observed.RestoreRootSHA256, + } + if restoreErr := restoreFailedPublication(ctx, item, reference, expectedRevisionFromPublished(item.observed), execution.authorize); restoreErr != nil { + return fmt.Errorf("canonical retry probe failed: %v; %w", err, restoreErr) + } + execution.result.Current-- + } + return err + } + } + if item.planned.Action != "noop" { + execution.recordDeployment(item, item.observed.NativeRevision) + } + return nil +} + +// publish switches one repository to its staged tree, proves the host serves +// exactly that tree, and restores the displaced revision if it does not. +func (execution *applyExecution) publish(item applyRepository) error { + ctx, request := execution.ctx, execution.request + // The publication locks are held across the whole pass, so Git cannot move + // the branch under it; confirming HEAD is enough and keeps the cost of a + // publication independent of how many repositories it covers. + if err := state.AssertGitHeadRevision(execution.root, execution.applyGitRevision); err != nil { + return err + } + if err := execution.authorize(item); err != nil { + return err + } + committed, err := item.host.Commit(ctx, item.hostRepository, item.hostStage, expectedRevisionFromPlan(item.planned)) + if err != nil { + return err + } + // Scoped to this repository, so a multi-repository apply does not keep every + // short-lived credential alive until the whole apply finishes. + if committed.Access.Credential != nil { + defer committed.Access.Credential.Destroy() + } + execution.result.Applied++ + canonical, observeErr := item.host.Observe(ctx, item.hostRepository) + if observeErr != nil || canonical.TreeSHA256 != item.planned.DesiredTreeSHA256 || canonical.NativeRevision != committed.Revision.NativeRevision || + (item.hostRepository.Type == "s3" && canonical != committed.Revision) { + probeErr := observeErr + if probeErr == nil { + probeErr = errors.New("canonical host observation does not match committed tree") + } + return execution.restore(item, committed, probeErr, "canonical probe failed") + } + if !request.StructuralOnly { + access := committed.Access + if access.Endpoint == "" { + access.Endpoint = committed.CanonicalEndpoint + } + if probeErr := verifyCanonicalClient(ctx, execution.root, item.repository, item.stage, access, request); probeErr != nil { + return execution.restore(item, committed, probeErr, "canonical client probe failed") + } + } + if err := state.AssertGitHeadRevision(execution.root, execution.applyGitRevision); err != nil { + return err + } + execution.recordDeployment(item, committed.Revision.NativeRevision) + return nil +} + +// restore compensates a failed probe and always reports the original failure, +// with the restore outcome attached. +func (execution *applyExecution) restore(item applyRepository, committed host.CommitResult, probeErr error, label string) error { + if committed.RestoreRef == nil { + return fmt.Errorf("%s: %w", label, probeErr) + } + if restoreErr := restoreFailedPublication(execution.ctx, item, *committed.RestoreRef, expectedRevisionFromPublished(committed.Revision), execution.authorize); restoreErr != nil { + return fmt.Errorf("%s: %v; %w", label, probeErr, restoreErr) + } + execution.result.Applied-- + return fmt.Errorf("%s: %w", label, probeErr) +} + +func (execution *applyExecution) recordDeployment(item applyRepository, nativeRevision string) { + execution.deployments = append(execution.deployments, deploymentRecordFor( + item.planned, item.deployment, item.observed, item.signingState, nativeRevision, + execution.plan.PlanID, execution.plan.Payload.CreatedAt, execution.request.currentTime(), + )) +} + +// applyPreparation revalidates the reviewed plan against the current workspace +// and builds each repository's stage. Nothing here has a publication effect: +// every failure leaves the hosts untouched. +type applyPreparation struct { + ctx context.Context + root string + request ApplyWorkspaceRequest + plan state.Plan + manifest state.Manifest + hosts host.Resolver + blobStore blob.Store + now time.Time + expiresAt time.Time + generatedAt time.Time + signatureTime time.Time + ledgerCommitted bool +} + +// prepareRepository checks one planned repository against the workspace it was +// planned from and, unless the host already serves the desired tree, rebuilds +// and verifies its stage. The caller owns cleanup of stages already built. +func (preparation *applyPreparation) prepareRepository(planned state.PlanRepository) (applyRepository, error) { + repository, exists := preparation.manifest.Repositories[planned.Name] + if !exists || repository.Format != planned.Format || repository.Gate != planned.Gate || !reflect.DeepEqual(repository.ApprovalKeys, planned.ApprovalKeys) || len(planned.Signing) > 1 || (len(planned.Signing) == 0) != (len(repository.SigningKeys) == 0) { + return applyRepository{}, fmt.Errorf("stale plan: repository %q configuration changed", planned.Name) + } + activeSigningKey, _, _, _, signingStateErr := repositorySigningState(repository) + if signingStateErr != nil { + return applyRepository{}, signingStateErr + } + for index, signing := range planned.Signing { + if index != 0 || signing.KeyName != activeSigningKey { + return applyRepository{}, fmt.Errorf("stale plan: repository %q signing key changed", planned.Name) + } + key, exists := preparation.manifest.Keys[signing.KeyName] + if !exists { + return applyRepository{}, fmt.Errorf("stale plan: repository %q signing key is missing", planned.Name) + } + keyExpiresAt, err := time.Parse(time.RFC3339, key.ExpiresAt) + if err != nil || preparation.expiresAt.After(keyExpiresAt) { + return applyRepository{}, fmt.Errorf("repository %q plan expires after its signing key", planned.Name) + } + if err := validateSigningRecipeMetadata(signing, repository.Suite); err != nil { + return applyRepository{}, fmt.Errorf("plan repository %q: %w", planned.Name, err) + } + } + hostIdentity, err := repositoryHostIdentity(repository) + if err != nil || hostIdentity != planned.HostIdentitySHA256 || repository.Host != planned.Host || repository.Visibility != planned.Visibility { + return applyRepository{}, fmt.Errorf("stale plan: repository %q host changed", planned.Name) + } + hostRepository := toHostRepository(preparation.root, preparation.manifest.Workspace.ID, hostIdentity, planned.Name, repository) + if hostRepository.CanonicalEndpoint != planned.CanonicalEndpoint { + return applyRepository{}, fmt.Errorf("stale plan: repository %q canonical endpoint changed", planned.Name) + } + installDocDigest, err := repositoryInstallDocDigest(preparation.root, planned.Name, repository) + if err != nil || installDocDigest != planned.InstallDocSHA256 { + return applyRepository{}, fmt.Errorf("stale plan: repository %q install document changed", planned.Name) + } + selectedHost, err := preparation.hosts.Resolve(preparation.ctx, hostRepository) + if err != nil { + return applyRepository{}, err + } + capabilities, err := selectedHost.Capabilities(preparation.ctx, hostRepository) + if err != nil { + return applyRepository{}, err + } + if capabilities.FaithfulPreview != planned.FaithfulPreview || capabilities.ConditionalCommit != planned.ConditionalCommit || capabilities.ConditionalRestore != planned.ConditionalRestore || + capabilities.PrivateRead != planned.PrivateRead || capabilities.CredentialBrokerIdentity != planned.CredentialBrokerIdentity { + return applyRepository{}, fmt.Errorf("stale plan: repository %q host capabilities changed", planned.Name) + } + lockPath, err := state.WorkspacePath(preparation.root, repository.Lock) + if err != nil { + return applyRepository{}, err + } + lockDigest, err := state.HashFile(lockPath) + if err != nil || lockDigest != planned.LockSHA256 { + return applyRepository{}, fmt.Errorf("stale plan: repository %q lock changed", planned.Name) + } + lock, err := state.LoadLock(preparation.root, repository) + if err != nil { + return applyRepository{}, err + } + if err := state.ValidateLock(lock, planned.Name, repository.Format); err != nil { + return applyRepository{}, err + } + ledger, err := state.LoadLedgerHistory(preparation.root, planned.Name) + if err != nil { + return applyRepository{}, err + } + if err := state.ValidatePublicationHistory(planned.Name, ledger); err != nil { + return applyRepository{}, err + } + if err := state.ValidatePublishedBindings(lock, ledger); err != nil { + return applyRepository{}, err + } + missingBindings := missingPublicationBindings(lock, repository, ledger) + expectedBindings := []state.PlanPublicationBinding(nil) + if planned.PublicationRecords { + expectedBindings = publicationBindingsForVersions(visiblePackageVersions(lock, repository)) + } + expectedAcquisitions := planAcquisitionsForVersions(visiblePackageVersions(lock, repository)) + if !reflect.DeepEqual(planned.Acquisitions, expectedAcquisitions) { + return applyRepository{}, fmt.Errorf("plan repository %q has inconsistent adopted acquisitions", planned.Name) + } + if planned.PublicationRecords != (len(planned.PublicationBindings) != 0) || + !reflect.DeepEqual(planned.PublicationBindings, expectedBindings) || + (!preparation.ledgerCommitted && planned.PublicationRecords != (len(missingBindings) != 0)) || + (preparation.ledgerCommitted && !planned.PublicationRecords && len(missingBindings) != 0) { + return applyRepository{}, fmt.Errorf("plan repository %q has inconsistent publication-record effects", planned.Name) + } + deployment, err := state.LoadDeployment(preparation.root, planned.Name) + if err != nil { + return applyRepository{}, err + } + desiredSigningState, err := repositoryDeploymentSigningState(repository, preparation.manifest.Keys) + if err != nil { + return applyRepository{}, err + } + observed, err := selectedHost.Observe(preparation.ctx, hostRepository) + if err != nil { + return applyRepository{}, err + } + plannedObserved := publishedFromPlanObservation(planned) + trustNotBefore := time.Time{} + if repository.SigningRotation != nil || deployment.SigningRotationPhase != "" { + trustNotBefore, err = state.AuthoritativeDeploymentTrustSince(preparation.root, planned.Name, deployment) + if err != nil { + return applyRepository{}, err + } + } + if err := validateRepositorySigningTransition(repository, preparation.manifest.Keys, deployment, plannedObserved, preparation.now, trustNotBefore); err != nil { + return applyRepository{}, fmt.Errorf("repository %q: %w", planned.Name, err) + } + matchesObserved := revisionMatchesPlanObservation(observed, planned) + managedRemote := repository.Host.Type == "s3" || repository.Host.Type == "github-pages" + matchesApplied := planned.Action != "noop" && observed.TreeSHA256 == planned.DesiredTreeSHA256 && + (!managedRemote || (observed.PlanID == preparation.plan.PlanID && observed.ChangeID == planned.ChangeID && observed.ManifestSHA256 == planned.DesiredManifestSHA256)) + deploymentApplied := deployment.PlanID == preparation.plan.PlanID && deployment.ChangeID == planned.ChangeID && deployment.TreeSHA256 == planned.DesiredTreeSHA256 && deployment.ManifestSHA256 == planned.DesiredManifestSHA256 && deployment.NativeRevision == observed.NativeRevision && deploymentSigningMatches(deployment, desiredSigningState) + deploymentCurrent := deploymentMatchesDesired(deployment, observed, planned.DesiredTreeSHA256, planned.DesiredManifestSHA256, desiredSigningState) + if !reflect.DeepEqual(deployment, planned.ObservedDeployment) && !deploymentApplied { + return applyRepository{}, fmt.Errorf("stale plan: repository %q deployment receipt changed", planned.Name) + } + if !matchesObserved && !matchesApplied { + if observed.TreeSHA256 == planned.ObservedTreeSHA256 { + return applyRepository{}, fmt.Errorf("stale plan: repository %q native revision changed", planned.Name) + } + if observed.TreeSHA256 == planned.DesiredTreeSHA256 { + return applyRepository{}, fmt.Errorf("stale plan: repository %q desired tree was published by another change", planned.Name) + } + return applyRepository{}, fmt.Errorf("stale plan: repository %q target changed", planned.Name) + } + expectedAction := "noop" + if planned.ObservedTreeSHA256 != planned.DesiredTreeSHA256 || (managedRemote && planned.ObservedManifestSHA256 != planned.DesiredManifestSHA256) || !publicationBindingsComplete(lock, repository, ledger) || !deploymentMatchesDesired(planned.ObservedDeployment, plannedObserved, planned.DesiredTreeSHA256, planned.DesiredManifestSHA256, desiredSigningState) { + expectedAction = "update" + if planned.ObservedRevision == "" { + expectedAction = "create" + } + } + if planned.Action != expectedAction || planned.ChangeID != planned.Name+":"+planned.DesiredTreeSHA256[:12] { + return applyRepository{}, fmt.Errorf("plan repository %q has inconsistent action metadata", planned.Name) + } + receiptRecovery := matchesApplied && reflect.DeepEqual(deployment, planned.ObservedDeployment) + current := observed.TreeSHA256 == planned.DesiredTreeSHA256 && (!managedRemote || observed.ManifestSHA256 == planned.DesiredManifestSHA256) && (deploymentApplied || (planned.Action == "noop" && deploymentCurrent) || receiptRecovery) + item := applyRepository{ + planned: planned, repository: repository, lock: lock, host: selectedHost, + hostRepository: hostRepository, observed: observed, current: current, deployment: deployment, signingState: desiredSigningState, + } + // Nothing to build: the host already serves this tree and no signing effect + // has to be replayed, so there is no stage for this repository. + if item.current && (preparation.request.StructuralOnly || planned.Action == "noop") && !(planned.Action != "noop" && len(planned.Signing) != 0) { + return item, nil + } + staging, err := stagingRoot(preparation.root) + if err != nil { + return applyRepository{}, err + } + stage, err := os.MkdirTemp(staging, ".snailmail-apply-*") + if err != nil { + return applyRepository{}, err + } + stageOutput := filepath.Join(stage, "repository") + staged, buildErr := buildLockedRepository(preparation.ctx, preparation.root, planned.Name, repository, lock, preparation.generatedAt, preparation.signatureTime, stageOutput, preparation.blobStore, preparation.manifest.Keys, planned.Signing, nil) + if buildErr == nil && (staged.TreeSHA256 != planned.DesiredTreeSHA256 || staged.ManifestSHA256 != planned.DesiredManifestSHA256) { + buildErr = errors.New("stale plan: rebuilt tree digest changed") + } + if buildErr == nil && !signingContentsEqual(staged.Signing, planned.Signing) { + buildErr = errors.New("stale plan: signing recipe changed") + } + var stagedManifest buildgraph.RepositoryManifest + if buildErr == nil { + structuralRequest := preparation.request + structuralRequest.StructuralOnly = true + stagedManifest, buildErr = verifyStaged(preparation.ctx, repository.Format, stageOutput, structuralRequest) + } + if buildErr != nil { + _ = os.RemoveAll(stage) + return applyRepository{}, buildErr + } + item.stageRoot, item.stage, item.stagedManifest = stage, stageOutput, stagedManifest + return item, nil +} From 53bcf0d0f2696f39279bd9c2294654c8ee1753c6 Mon Sep 17 00:00:00 2001 From: Polina Simonenko Date: Tue, 28 Jul 2026 13:48:26 +0200 Subject: [PATCH 18/22] collapse repeated flag boilerplate in the CLI --- cmd/snailmail/flags.go | 88 ++++++++++++++++ cmd/snailmail/main.go | 228 ++++++++++++----------------------------- 2 files changed, 152 insertions(+), 164 deletions(-) create mode 100644 cmd/snailmail/flags.go diff --git a/cmd/snailmail/flags.go b/cmd/snailmail/flags.go new file mode 100644 index 0000000..ec8c585 --- /dev/null +++ b/cmd/snailmail/flags.go @@ -0,0 +1,88 @@ +package main + +import ( + "errors" + "flag" + "fmt" + "io" + "time" +) + +// commandFlags removes the boilerplate every subcommand repeated: routing usage +// output to stderr, declaring the shared --workspace flag, and rejecting +// positional arguments the command does not take. +type commandFlags struct { + set *flag.FlagSet + workspace *string +} + +func newCommandFlags(name string, stderr io.Writer) *commandFlags { + set := flag.NewFlagSet(name, flag.ContinueOnError) + set.SetOutput(stderr) + return &commandFlags{set: set} +} + +// withWorkspace declares the shared workspace root flag. +func (flags *commandFlags) withWorkspace() *commandFlags { + flags.workspace = flags.set.String("workspace", ".", "workspace root") + return flags +} + +func (flags *commandFlags) String(name, value, usage string) *string { + return flags.set.String(name, value, usage) +} + +func (flags *commandFlags) Bool(name string, value bool, usage string) *bool { + return flags.set.Bool(name, value, usage) +} + +func (flags *commandFlags) Int(name string, value int, usage string) *int { + return flags.set.Int(name, value, usage) +} + +func (flags *commandFlags) Int64(name string, value int64, usage string) *int64 { + return flags.set.Int64(name, value, usage) +} + +func (flags *commandFlags) Duration(name string, value time.Duration, usage string) *time.Duration { + return flags.set.Duration(name, value, usage) +} + +// Root is the resolved workspace root, or "." for commands without the flag. +func (flags *commandFlags) Root() string { + if flags.workspace == nil { + return "." + } + return *flags.workspace +} + +// Parse, NArg, Arg and Args pass through for the commands that take positional +// arguments and validate the count themselves. +func (flags *commandFlags) Parse(arguments []string) error { return flags.set.Parse(arguments) } +func (flags *commandFlags) NArg() int { return flags.set.NArg() } +func (flags *commandFlags) Arg(index int) string { return flags.set.Arg(index) } +func (flags *commandFlags) Args() []string { return flags.set.Args() } + +// parse reads arguments and rejects any positional argument, which every +// command using it treats as a usage error. +func (flags *commandFlags) parse(arguments []string) error { + if err := flags.set.Parse(arguments); err != nil { + return err + } + if flags.set.NArg() != 0 { + return fmt.Errorf("unexpected argument %q", flags.set.Arg(0)) + } + return nil +} + +// parseWithArguments reads arguments and requires exactly count positional +// arguments, reporting usage when the count does not match. +func (flags *commandFlags) parseWithArguments(arguments []string, count int, usage string) ([]string, error) { + if err := flags.set.Parse(arguments); err != nil { + return nil, err + } + if flags.set.NArg() != count { + return nil, errors.New(usage) + } + return flags.set.Args(), nil +} diff --git a/cmd/snailmail/main.go b/cmd/snailmail/main.go index d32e74b..3a0465c 100644 --- a/cmd/snailmail/main.go +++ b/cmd/snailmail/main.go @@ -4,7 +4,6 @@ import ( "context" "encoding/json" "errors" - "flag" "fmt" "io" "net" @@ -91,18 +90,13 @@ func run(ctx context.Context, args []string, stdout, stderr io.Writer) error { } func runInit(args []string, stdout, stderr io.Writer) error { - flags := flag.NewFlagSet("init", flag.ContinueOnError) - flags.SetOutput(stderr) - workspace := flags.String("workspace", ".", "workspace root") + flags := newCommandFlags("init", stderr).withWorkspace() name := flags.String("name", "", "workspace name") forgeRepository := flags.String("forge-repo", "", "GitHub state repository (owner/name) for PR gates") - if err := flags.Parse(args); err != nil { + if err := flags.parse(args); err != nil { return err } - if flags.NArg() != 0 { - return fmt.Errorf("unexpected argument %q", flags.Arg(0)) - } - if err := engine.InitWorkspace(engine.InitWorkspaceRequest{Root: *workspace, Name: *name, ForgeRepository: *forgeRepository}); err != nil { + if err := engine.InitWorkspace(engine.InitWorkspaceRequest{Root: flags.Root(), Name: *name, ForgeRepository: *forgeRepository}); err != nil { return err } printBrand(stdout) @@ -115,9 +109,7 @@ func runSetup(args []string, stdout, stderr io.Writer) error { return errors.New("usage: snailmail setup --name NAME --host [host options]") } format := args[0] - flags := flag.NewFlagSet("setup "+format, flag.ContinueOnError) - flags.SetOutput(stderr) - workspace := flags.String("workspace", ".", "workspace root") + flags := newCommandFlags("setup "+format, stderr).withWorkspace() name := flags.String("name", "", "repository name") output := flags.String("output", "", "workspace-relative published directory") hostType := flags.String("host", "local", "host type: local, s3, or github-pages") @@ -143,16 +135,13 @@ func runSetup(args []string, stdout, stderr io.Writer) error { suite := flags.String("suite", "stable", "Debian suite") component := flags.String("component", "main", "Debian component") architectures := flags.String("architectures", "amd64", "comma-separated Debian architectures") - if err := flags.Parse(args[1:]); err != nil { + if err := flags.parse(args[1:]); err != nil { return err } - if flags.NArg() != 0 { - return fmt.Errorf("unexpected argument %q", flags.Arg(0)) - } resolvedApprovalKeys := splitList(*approvalKeys) sort.Strings(resolvedApprovalKeys) if err := engine.SetupRepository(engine.SetupRepositoryRequest{ - Root: *workspace, Name: *name, Format: format, Output: *output, + Root: flags.Root(), Name: *name, Format: format, Output: *output, HostType: *hostType, Visibility: *visibility, Gate: *gatePolicy, ApprovalKeys: resolvedApprovalKeys, SigningKey: *signingKey, AllowUnsigned: *allowUnsigned, Bucket: *bucket, Prefix: *prefix, Region: *region, Endpoint: *endpoint, CanonicalEndpoint: *canonicalEndpoint, UsePathStyle: *usePathStyle, @@ -183,22 +172,17 @@ func runKeys(ctx context.Context, args []string, stdout, stderr io.Writer) error return errors.New("usage: snailmail keys new NAME [--algo openpgp-rsa4096] [--expires-in 17520h]") } name := args[1] - flags := flag.NewFlagSet("keys new", flag.ContinueOnError) - flags.SetOutput(stderr) - workspace := flags.String("workspace", ".", "workspace root") + flags := newCommandFlags("keys new", stderr).withWorkspace() algorithm := flags.String("algo", "openpgp-rsa4096", "signing algorithm") expiresIn := flags.Duration("expires-in", 2*365*24*time.Hour, "key validity duration") - if err := flags.Parse(args[2:]); err != nil { + if err := flags.parse(args[2:]); err != nil { return err } - if flags.NArg() != 0 { - return fmt.Errorf("unexpected argument %q", flags.Arg(0)) - } store, err := wire.NewSignerStore() if err != nil { return err } - result, err := engine.NewKey(ctx, engine.NewKeyRequest{Root: *workspace, Name: name, Algorithm: *algorithm, ExpiresIn: *expiresIn, Keys: store}) + result, err := engine.NewKey(ctx, engine.NewKeyRequest{Root: flags.Root(), Name: name, Algorithm: *algorithm, ExpiresIn: *expiresIn, Keys: store}) if err != nil { return err } @@ -212,20 +196,15 @@ func runKeys(ctx context.Context, args []string, stdout, stderr io.Writer) error return errors.New("usage: snailmail keys publish NAME") } name := args[1] - flags := flag.NewFlagSet("keys publish", flag.ContinueOnError) - flags.SetOutput(stderr) - workspace := flags.String("workspace", ".", "workspace root") - if err := flags.Parse(args[2:]); err != nil { + flags := newCommandFlags("keys publish", stderr).withWorkspace() + if err := flags.parse(args[2:]); err != nil { return err } - if flags.NArg() != 0 { - return fmt.Errorf("unexpected argument %q", flags.Arg(0)) - } store, err := wire.NewSignerStore() if err != nil { return err } - result, err := engine.PublishKey(ctx, engine.PublishKeyRequest{Root: *workspace, Name: name, Keys: store}) + result, err := engine.PublishKey(ctx, engine.PublishKeyRequest{Root: flags.Root(), Name: name, Keys: store}) if err != nil { return err } @@ -238,20 +217,15 @@ func runKeys(ctx context.Context, args []string, stdout, stderr io.Writer) error return errors.New("usage: snailmail keys rotate REPOSITORY --successor KEY [--minimum-refresh 720h] | --advance --yes") } repository := args[1] - flags := flag.NewFlagSet("keys rotate", flag.ContinueOnError) - flags.SetOutput(stderr) - workspace := flags.String("workspace", ".", "workspace root") + flags := newCommandFlags("keys rotate", stderr).withWorkspace() successor := flags.String("successor", "", "successor signing key name") advance := flags.Bool("advance", false, "advance the deployed rotation to its next phase") minimumRefresh := flags.Duration("minimum-refresh", 30*24*time.Hour, "minimum client keyring refresh window") expiresIn := flags.Duration("expires-in", 2*365*24*time.Hour, "new successor key validity") confirmed := flags.Bool("yes", false, "confirm an advance transition") - if err := flags.Parse(args[2:]); err != nil { + if err := flags.parse(args[2:]); err != nil { return err } - if flags.NArg() != 0 { - return fmt.Errorf("unexpected argument %q", flags.Arg(0)) - } if *advance && !*confirmed { return errors.New("keys rotate --advance requires --yes") } @@ -267,7 +241,7 @@ func runKeys(ctx context.Context, args []string, stdout, stderr io.Writer) error } } result, err := engine.RotateKey(ctx, engine.RotateKeyRequest{ - Root: *workspace, Repository: repository, Successor: *successor, Advance: *advance, + Root: flags.Root(), Repository: repository, Successor: *successor, Advance: *advance, MinimumRefresh: *minimumRefresh, ExpiresIn: *expiresIn, Keys: keyGenerator, }) if err != nil { @@ -284,16 +258,11 @@ func runKeys(ctx context.Context, args []string, stdout, stderr io.Writer) error } return nil case "audit": - flags := flag.NewFlagSet("keys audit", flag.ContinueOnError) - flags.SetOutput(stderr) - workspace := flags.String("workspace", ".", "workspace root") - if err := flags.Parse(args[1:]); err != nil { + flags := newCommandFlags("keys audit", stderr).withWorkspace() + if err := flags.parse(args[1:]); err != nil { return err } - if flags.NArg() != 0 { - return fmt.Errorf("unexpected argument %q", flags.Arg(0)) - } - result, err := engine.AuditKeys(engine.PublishKeyRequest{Root: *workspace}, time.Time{}) + result, err := engine.AuditKeys(engine.PublishKeyRequest{Root: flags.Root()}, time.Time{}) if err != nil { return err } @@ -325,9 +294,7 @@ func runKeys(ctx context.Context, args []string, stdout, stderr io.Writer) error } func runAdd(ctx context.Context, args []string, stdout, stderr io.Writer) error { - flags := flag.NewFlagSet("add", flag.ContinueOnError) - flags.SetOutput(stderr) - workspace := flags.String("workspace", ".", "workspace root") + flags := newCommandFlags("add", stderr).withWorkspace() track := flags.String("track", "stable", "placement track") if err := flags.Parse(args); err != nil { return err @@ -336,7 +303,7 @@ func runAdd(ctx context.Context, args []string, stdout, stderr io.Writer) error return errors.New("usage: snailmail add [--track stable] REPOSITORY ARTIFACT...") } result, err := engine.AddArtifacts(engine.AddArtifactsRequest{ - Context: ctx, Root: *workspace, Repository: flags.Arg(0), Artifacts: flags.Args()[1:], Track: *track, + Context: ctx, Root: flags.Root(), Repository: flags.Arg(0), Artifacts: flags.Args()[1:], Track: *track, Blobs: wire.NewBlobResolver(), }) if err != nil { @@ -355,9 +322,7 @@ func runAdd(ctx context.Context, args []string, stdout, stderr io.Writer) error } func runPromote(args []string, stdout, stderr io.Writer) error { - flags := flag.NewFlagSet("promote", flag.ContinueOnError) - flags.SetOutput(stderr) - workspace := flags.String("workspace", ".", "workspace root") + flags := newCommandFlags("promote", stderr).withWorkspace() track := flags.String("track", "stable", "destination placement track") distro := flags.String("distro", "", "Debian placement distro (defaults to repository suite)") if err := flags.Parse(args); err != nil { @@ -367,7 +332,7 @@ func runPromote(args []string, stdout, stderr io.Writer) error { return errors.New("usage: snailmail promote [--track stable] [--distro DISTRO] REPOSITORY PACKAGE VERSION") } result, err := engine.Promote(engine.PlacementMutationRequest{ - Root: *workspace, Repository: flags.Arg(0), Package: flags.Arg(1), Version: flags.Arg(2), Track: *track, Distro: *distro, + Root: flags.Root(), Repository: flags.Arg(0), Package: flags.Arg(1), Version: flags.Arg(2), Track: *track, Distro: *distro, }) if err != nil { return err @@ -382,9 +347,7 @@ func runPromote(args []string, stdout, stderr io.Writer) error { } func runYank(args []string, stdout, stderr io.Writer) error { - flags := flag.NewFlagSet("yank", flag.ContinueOnError) - flags.SetOutput(stderr) - workspace := flags.String("workspace", ".", "workspace root") + flags := newCommandFlags("yank", stderr).withWorkspace() track := flags.String("track", "", "placement track to remove") distro := flags.String("distro", "", "Debian placement distro (defaults to repository suite)") all := flags.Bool("all", false, "remove every placement for the package version") @@ -395,7 +358,7 @@ func runYank(args []string, stdout, stderr io.Writer) error { return errors.New("usage: snailmail yank (--track TRACK [--distro DISTRO] | --all) REPOSITORY PACKAGE VERSION") } result, err := engine.Yank(engine.PlacementMutationRequest{ - Root: *workspace, Repository: flags.Arg(0), Package: flags.Arg(1), Version: flags.Arg(2), Track: *track, Distro: *distro, All: *all, + Root: flags.Root(), Repository: flags.Arg(0), Package: flags.Arg(1), Version: flags.Arg(2), Track: *track, Distro: *distro, All: *all, }) if err != nil { return err @@ -416,9 +379,7 @@ func runPrune(args []string, stdout, stderr io.Writer) error { return errors.New("usage: snailmail prune REPOSITORY --keep N") } repository := args[0] - flags := flag.NewFlagSet("prune", flag.ContinueOnError) - flags.SetOutput(stderr) - workspace := flags.String("workspace", ".", "workspace root") + flags := newCommandFlags("prune", stderr).withWorkspace() keep := flags.Int("keep", 0, "versions to retain per package, track, and distro") if err := flags.Parse(args[1:]); err != nil { return err @@ -426,7 +387,7 @@ func runPrune(args []string, stdout, stderr io.Writer) error { if flags.NArg() != 0 || *keep < 1 { return errors.New("usage: snailmail prune REPOSITORY --keep N") } - result, err := engine.Prune(engine.PruneRequest{Root: *workspace, Repository: repository, Keep: *keep}) + result, err := engine.Prune(engine.PruneRequest{Root: flags.Root(), Repository: repository, Keep: *keep}) if err != nil { return err } @@ -441,9 +402,7 @@ func runPrune(args []string, stdout, stderr io.Writer) error { } func runCheck(ctx context.Context, args []string, stdout, stderr io.Writer) error { - flags := flag.NewFlagSet("check", flag.ContinueOnError) - flags.SetOutput(stderr) - workspace := flags.String("workspace", ".", "workspace root") + flags := newCommandFlags("check", stderr).withWorkspace() origins := flags.Bool("origins", false, "re-fetch recorded adopted origins") maxOrigins := flags.Int("max-origins", 2, "maximum recorded origins to re-fetch (1-4)") originOffset := flags.Int("origin-offset", 0, "skip this many sorted recorded origins") @@ -453,7 +412,7 @@ func runCheck(ctx context.Context, args []string, stdout, stderr io.Writer) erro if flags.NArg() != 0 { return errors.New("usage: snailmail check [--workspace DIR] [--origins --max-origins N --origin-offset N]") } - result, err := engine.CheckWorkspace(ctx, engine.CheckWorkspaceRequest{Root: *workspace, Blobs: wire.NewBlobResolver(), Origins: *origins, Sources: httpsource.New(), MaxOrigins: *maxOrigins, OriginOffset: *originOffset}) + result, err := engine.CheckWorkspace(ctx, engine.CheckWorkspaceRequest{Root: flags.Root(), Blobs: wire.NewBlobResolver(), Origins: *origins, Sources: httpsource.New(), MaxOrigins: *maxOrigins, OriginOffset: *originOffset}) if err != nil { return err } @@ -475,9 +434,7 @@ func runCheck(ctx context.Context, args []string, stdout, stderr io.Writer) erro } func runStatus(ctx context.Context, args []string, stdout, stderr io.Writer) error { - flags := flag.NewFlagSet("status", flag.ContinueOnError) - flags.SetOutput(stderr) - workspace := flags.String("workspace", ".", "workspace root") + flags := newCommandFlags("status", stderr).withWorkspace() jsonOutput := flags.Bool("json", false, "emit machine-readable JSON") if err := flags.Parse(args); err != nil { return err @@ -485,7 +442,7 @@ func runStatus(ctx context.Context, args []string, stdout, stderr io.Writer) err if flags.NArg() != 0 { return errors.New("usage: snailmail status [--workspace DIR] [--json]") } - result, err := engine.StatusWorkspace(ctx, engine.StatusWorkspaceRequest{Root: *workspace}) + result, err := engine.StatusWorkspace(ctx, engine.StatusWorkspaceRequest{Root: flags.Root()}) if err != nil { return err } @@ -509,8 +466,7 @@ func runDoctor(ctx context.Context, args []string, stdout, stderr io.Writer) err } func runDoctorWithFetcher(ctx context.Context, args []string, stdout, stderr io.Writer, fetcher source.Fetcher) error { - flags := flag.NewFlagSet("doctor", flag.ContinueOnError) - flags.SetOutput(stderr) + flags := newCommandFlags("doctor", stderr) format := flags.String("format", "auto", "repository format: auto, pypi, deb, or helm") project := flags.String("project", "", "PyPI project to inspect") suite := flags.String("suite", "", "Debian suite") @@ -561,9 +517,7 @@ func runAdopt(ctx context.Context, args []string, stdout, stderr io.Writer) erro } func runAdoptWithFetcher(ctx context.Context, args []string, stdout, stderr io.Writer, fetcher source.Fetcher) error { - flags := flag.NewFlagSet("adopt", flag.ContinueOnError) - flags.SetOutput(stderr) - workspace := flags.String("workspace", ".", "workspace root") + flags := newCommandFlags("adopt", stderr).withWorkspace() digest := flags.String("sha256", "", "required artifact SHA-256 pin") filename := flags.String("filename", "", "artifact filename override") track := flags.String("track", "", "placement track") @@ -578,7 +532,7 @@ func runAdoptWithFetcher(ctx context.Context, args []string, stdout, stderr io.W return errors.New("usage: snailmail adopt --sha256 HEX --public-origin [options] REPOSITORY URL") } result, err := engine.AdoptArtifact(ctx, engine.AdoptArtifactRequest{ - Root: *workspace, Repository: flags.Arg(0), URL: flags.Arg(1), SHA256: *digest, + Root: flags.Root(), Repository: flags.Arg(0), URL: flags.Arg(1), SHA256: *digest, Filename: *filename, Track: *track, Distro: *distro, DryRun: *dryRun, PublicOrigin: *publicOrigin, Fetcher: fetcher, }) if err != nil { @@ -606,22 +560,17 @@ func runBlobStore(ctx context.Context, args []string, stdout, stderr io.Writer) return errors.New("usage: snailmail blob-store [options]") } storeType := args[0] - flags := flag.NewFlagSet("blob-store "+storeType, flag.ContinueOnError) - flags.SetOutput(stderr) - workspace := flags.String("workspace", ".", "workspace root") + flags := newCommandFlags("blob-store "+storeType, stderr).withWorkspace() bucket := flags.String("bucket", "", "S3 blob bucket") prefix := flags.String("prefix", "", "S3 blob prefix") region := flags.String("region", "", "AWS region") endpoint := flags.String("endpoint", "", "optional S3 API endpoint") usePathStyle := flags.Bool("use-path-style", false, "use path-style S3 requests") - if err := flags.Parse(args[1:]); err != nil { + if err := flags.parse(args[1:]); err != nil { return err } - if flags.NArg() != 0 { - return fmt.Errorf("unexpected argument %q", flags.Arg(0)) - } if err := engine.ConfigureBlobStore(ctx, engine.ConfigureBlobStoreRequest{ - Root: *workspace, Type: storeType, Bucket: *bucket, Prefix: *prefix, Region: *region, + Root: flags.Root(), Type: storeType, Bucket: *bucket, Prefix: *prefix, Region: *region, Endpoint: *endpoint, UsePathStyle: *usePathStyle, Blobs: wire.NewBlobResolver(), }); err != nil { return err @@ -633,19 +582,14 @@ func runBlobStore(ctx context.Context, args []string, stdout, stderr io.Writer) } func runPlan(ctx context.Context, args []string, stdout, stderr io.Writer) error { - flags := flag.NewFlagSet("plan", flag.ContinueOnError) - flags.SetOutput(stderr) - workspace := flags.String("workspace", ".", "workspace root") + flags := newCommandFlags("plan", stderr).withWorkspace() output := flags.String("out", "snailmail.snailmail-plan.json", "plan output file") generatedAtValue := flags.String("generated-at", "", "explicit RFC3339 repository generation time") expires := flags.Duration("expires", 2*time.Hour, "plan lifetime") structuralOnly := flags.Bool("structural-only", false, "review a plan without ecosystem client verification") - if err := flags.Parse(args); err != nil { + if err := flags.parse(args); err != nil { return err } - if flags.NArg() != 0 { - return fmt.Errorf("unexpected argument %q", flags.Arg(0)) - } generatedAt, err := optionalTime(*generatedAtValue) if err != nil { return err @@ -657,7 +601,7 @@ func runPlan(ctx context.Context, args []string, stdout, stderr io.Writer) error return err } result, err := engine.PlanWorkspace(ctx, engine.PlanWorkspaceRequest{ - Root: *workspace, Output: *output, GeneratedAt: generatedAt, ExpiresIn: *expires, + Root: flags.Root(), Output: *output, GeneratedAt: generatedAt, ExpiresIn: *expires, Hosts: hosts, Blobs: wire.NewBlobResolver(), Signers: signers, VerificationMode: verificationMode(*structuralOnly), }) if err != nil { @@ -675,9 +619,7 @@ func runPlan(ctx context.Context, args []string, stdout, stderr io.Writer) error } func runApply(ctx context.Context, args []string, stdout, stderr io.Writer) error { - flags := flag.NewFlagSet("apply", flag.ContinueOnError) - flags.SetOutput(stderr) - workspace := flags.String("workspace", ".", "workspace root") + flags := newCommandFlags("apply", stderr).withWorkspace() plan := flags.String("plan", "snailmail.snailmail-plan.json", "reviewed plan file") structuralOnly := flags.Bool("structural-only", false, "skip ecosystem client verification") python := flags.String("python", "python3", "Python executable for PyPI verification") @@ -686,12 +628,9 @@ func runApply(ctx context.Context, args []string, stdout, stderr io.Writer) erro helmImage := flags.String("helm-image", engine.DefaultHelmVerificationImage, "digest-pinned Helm image") maxWorkspaceMiB := flags.Int64("max-workspace-mib", 4096, "maximum Debian verification workspace in MiB") approvalFile := flags.String("approvals", "", "approval evidence file (defaults beside plan)") - if err := flags.Parse(args); err != nil { + if err := flags.parse(args); err != nil { return err } - if flags.NArg() != 0 { - return fmt.Errorf("unexpected argument %q", flags.Arg(0)) - } hosts := wire.NewHostResolver() defer hosts.Close() resolvedApprovalFile := *approvalFile @@ -699,10 +638,10 @@ func runApply(ctx context.Context, args []string, stdout, stderr io.Writer) erro resolvedApprovalFile = *plan + ".approvals.json" } if !filepath.IsAbs(resolvedApprovalFile) { - resolvedApprovalFile = filepath.Join(*workspace, resolvedApprovalFile) + resolvedApprovalFile = filepath.Join(flags.Root(), resolvedApprovalFile) } result, err := engine.ApplyWorkspace(ctx, engine.ApplyWorkspaceRequest{ - Root: *workspace, Plan: *plan, StructuralOnly: *structuralOnly, Python: *python, Runner: *runner, + Root: flags.Root(), Plan: *plan, StructuralOnly: *structuralOnly, Python: *python, Runner: *runner, DebianImage: *debianImage, HelmImage: *helmImage, MaxWorkspaceBytes: *maxWorkspaceMiB << 20, Hosts: hosts, Blobs: wire.NewBlobResolver(), Gates: gate.NewDefaultEvaluator(resolvedApprovalFile), }) @@ -728,26 +667,21 @@ func runApply(ctx context.Context, args []string, stdout, stderr io.Writer) erro } func runApprove(args []string, stdout, stderr io.Writer) error { - flags := flag.NewFlagSet("approve", flag.ContinueOnError) - flags.SetOutput(stderr) - workspace := flags.String("workspace", ".", "workspace root") + flags := newCommandFlags("approve", stderr).withWorkspace() plan := flags.String("plan", "snailmail.snailmail-plan.json", "reviewed plan file") output := flags.String("out", "", "approval evidence output") repository := flags.String("repository", "", "repository to approve") keyFile := flags.String("key", "", "Ed25519 approval private key file") expires := flags.Duration("expires", 30*time.Minute, "approval lifetime") yes := flags.Bool("yes", false, "confirm approval of the exact plan ID") - if err := flags.Parse(args); err != nil { + if err := flags.parse(args); err != nil { return err } - if flags.NArg() != 0 { - return fmt.Errorf("unexpected argument %q", flags.Arg(0)) - } if !*yes { return errors.New("approval requires --yes after reviewing the exact plan") } result, err := engine.ApprovePlan(engine.ApprovePlanRequest{ - Root: *workspace, Plan: *plan, Output: *output, Repository: *repository, + Root: flags.Root(), Plan: *plan, Output: *output, Repository: *repository, KeyFile: *keyFile, ExpiresIn: *expires, }) if err != nil { @@ -762,8 +696,7 @@ func runApprove(args []string, stdout, stderr io.Writer) error { } func runApprovalKey(args []string, stdout, stderr io.Writer) error { - flags := flag.NewFlagSet("approval-key", flag.ContinueOnError) - flags.SetOutput(stderr) + flags := newCommandFlags("approval-key", stderr) output := flags.String("out", "", "private key output file") if err := flags.Parse(args); err != nil { return err @@ -783,18 +716,13 @@ func runApprovalKey(args []string, stdout, stderr io.Writer) error { } func runRender(args []string, stdout, stderr io.Writer) error { - flags := flag.NewFlagSet("render", flag.ContinueOnError) - flags.SetOutput(stderr) - workspace := flags.String("workspace", ".", "workspace root") + flags := newCommandFlags("render", stderr).withWorkspace() output := flags.String("output", "site", "status site output directory") plan := flags.String("plan", "snailmail.snailmail-plan.json", "optional plan used for pending gates") - if err := flags.Parse(args); err != nil { + if err := flags.parse(args); err != nil { return err } - if flags.NArg() != 0 { - return fmt.Errorf("unexpected argument %q", flags.Arg(0)) - } - result, err := engine.RenderStatus(engine.RenderStatusRequest{Root: *workspace, Output: *output, Plan: *plan}) + result, err := engine.RenderStatus(engine.RenderStatusRequest{Root: flags.Root(), Output: *output, Plan: *plan}) if err != nil { return err } @@ -821,17 +749,13 @@ func runBuild(ctx context.Context, args []string, stdout, stderr io.Writer) erro } func runBuildPyPI(ctx context.Context, args []string, stdout, stderr io.Writer) error { - flags := flag.NewFlagSet("build pypi", flag.ContinueOnError) - flags.SetOutput(stderr) + flags := newCommandFlags("build pypi", stderr) input := flags.String("input", "", "directory containing wheels and source distributions") output := flags.String("output", "", "directory to write the static repository") generatedAtValue := flags.String("generated-at", defaultGeneratedAt, "explicit RFC3339 generation time") - if err := flags.Parse(args); err != nil { + if err := flags.parse(args); err != nil { return err } - if flags.NArg() != 0 { - return fmt.Errorf("unexpected argument %q", flags.Arg(0)) - } generatedAt, err := time.Parse(time.RFC3339, *generatedAtValue) if err != nil { return fmt.Errorf("parse --generated-at: %w", err) @@ -852,20 +776,16 @@ func runBuildPyPI(ctx context.Context, args []string, stdout, stderr io.Writer) } func runBuildDeb(ctx context.Context, args []string, stdout, stderr io.Writer) error { - flags := flag.NewFlagSet("build deb", flag.ContinueOnError) - flags.SetOutput(stderr) + flags := newCommandFlags("build deb", stderr) input := flags.String("input", "", "directory containing Debian packages") output := flags.String("output", "", "directory to write the static repository") suite := flags.String("suite", "stable", "Debian suite/codename") component := flags.String("component", "main", "Debian component") architecturesValue := flags.String("architectures", "amd64", "comma-separated target architectures") generatedAtValue := flags.String("generated-at", defaultGeneratedAt, "explicit RFC3339 generation time") - if err := flags.Parse(args); err != nil { + if err := flags.parse(args); err != nil { return err } - if flags.NArg() != 0 { - return fmt.Errorf("unexpected argument %q", flags.Arg(0)) - } generatedAt, err := time.Parse(time.RFC3339, *generatedAtValue) if err != nil { return fmt.Errorf("parse --generated-at: %w", err) @@ -889,17 +809,13 @@ func runBuildDeb(ctx context.Context, args []string, stdout, stderr io.Writer) e } func runBuildHelm(ctx context.Context, args []string, stdout, stderr io.Writer) error { - flags := flag.NewFlagSet("build helm", flag.ContinueOnError) - flags.SetOutput(stderr) + flags := newCommandFlags("build helm", stderr) input := flags.String("input", "", "directory containing packaged Helm charts") output := flags.String("output", "", "directory to write the static repository") generatedAtValue := flags.String("generated-at", defaultGeneratedAt, "explicit RFC3339 generation time") - if err := flags.Parse(args); err != nil { + if err := flags.parse(args); err != nil { return err } - if flags.NArg() != 0 { - return fmt.Errorf("unexpected argument %q", flags.Arg(0)) - } generatedAt, err := time.Parse(time.RFC3339, *generatedAtValue) if err != nil { return fmt.Errorf("parse --generated-at: %w", err) @@ -932,17 +848,13 @@ func runVerify(ctx context.Context, args []string, stdout, stderr io.Writer) err } func runVerifyPyPI(ctx context.Context, args []string, stdout, stderr io.Writer) error { - flags := flag.NewFlagSet("verify pypi", flag.ContinueOnError) - flags.SetOutput(stderr) + flags := newCommandFlags("verify pypi", stderr) repository := flags.String("repo", "", "generated repository directory") python := flags.String("python", "python3", "Python executable whose pip client verifies installs") structuralOnly := flags.Bool("structural-only", false, "verify files and indexes without invoking pip") - if err := flags.Parse(args); err != nil { + if err := flags.parse(args); err != nil { return err } - if flags.NArg() != 0 { - return fmt.Errorf("unexpected argument %q", flags.Arg(0)) - } result, err := engine.VerifyPyPI(ctx, engine.VerifyPyPIRequest{ Repository: *repository, Python: *python, @@ -963,19 +875,15 @@ func runVerifyPyPI(ctx context.Context, args []string, stdout, stderr io.Writer) } func runVerifyDeb(ctx context.Context, args []string, stdout, stderr io.Writer) error { - flags := flag.NewFlagSet("verify deb", flag.ContinueOnError) - flags.SetOutput(stderr) + flags := newCommandFlags("verify deb", stderr) repository := flags.String("repo", "", "generated repository directory") runner := flags.String("runner", "podman", "OCI runner executable") image := flags.String("image", engine.DefaultDebianVerificationImage, "digest-pinned Debian client image") maxWorkspaceMiB := flags.Int64("max-workspace-mib", 4096, "maximum temporary verification workspace in MiB") structuralOnly := flags.Bool("structural-only", false, "verify files and indexes without invoking apt") - if err := flags.Parse(args); err != nil { + if err := flags.parse(args); err != nil { return err } - if flags.NArg() != 0 { - return fmt.Errorf("unexpected argument %q", flags.Arg(0)) - } result, err := engine.VerifyDeb(ctx, engine.VerifyDebRequest{ Repository: *repository, Runner: *runner, @@ -998,18 +906,14 @@ func runVerifyDeb(ctx context.Context, args []string, stdout, stderr io.Writer) } func runVerifyHelm(ctx context.Context, args []string, stdout, stderr io.Writer) error { - flags := flag.NewFlagSet("verify helm", flag.ContinueOnError) - flags.SetOutput(stderr) + flags := newCommandFlags("verify helm", stderr) repository := flags.String("repo", "", "generated repository directory") runner := flags.String("runner", "podman", "OCI runner executable") image := flags.String("image", engine.DefaultHelmVerificationImage, "digest-pinned Helm client image") structuralOnly := flags.Bool("structural-only", false, "verify files and index without invoking Helm") - if err := flags.Parse(args); err != nil { + if err := flags.parse(args); err != nil { return err } - if flags.NArg() != 0 { - return fmt.Errorf("unexpected argument %q", flags.Arg(0)) - } result, err := engine.VerifyHelm(ctx, engine.VerifyHelmRequest{Repository: *repository, Runner: *runner, Image: *image, StructuralOnly: *structuralOnly}) if err != nil { return err @@ -1026,16 +930,12 @@ func runVerifyHelm(ctx context.Context, args []string, stdout, stderr io.Writer) } func runServe(ctx context.Context, args []string, stdout, stderr io.Writer) error { - flags := flag.NewFlagSet("serve", flag.ContinueOnError) - flags.SetOutput(stderr) + flags := newCommandFlags("serve", stderr) repository := flags.String("repo", "", "generated repository directory") listenAddress := flags.String("listen", "127.0.0.1:8080", "HTTP listen address") - if err := flags.Parse(args); err != nil { + if err := flags.parse(args); err != nil { return err } - if flags.NArg() != 0 { - return fmt.Errorf("unexpected argument %q", flags.Arg(0)) - } absolute, err := filepath.Abs(*repository) if err != nil { return fmt.Errorf("resolve repository: %w", err) From e6c98d31fa1dd4da3bbc2dd30d22aa8db13285ea Mon Sep 17 00:00:00 2001 From: Polina Simonenko Date: Tue, 28 Jul 2026 13:50:37 +0200 Subject: [PATCH 19/22] resolve a workspace root once and let os.Root guard the blob path --- internal/state/cas.go | 18 +++---- internal/state/cas_containment_test.go | 66 ++++++++++++++++++++++++++ internal/state/store.go | 32 ++++++++++++- 3 files changed, 107 insertions(+), 9 deletions(-) create mode 100644 internal/state/cas_containment_test.go diff --git a/internal/state/cas.go b/internal/state/cas.go index 894d8c1..3b96caa 100644 --- a/internal/state/cas.go +++ b/internal/state/cas.go @@ -195,12 +195,17 @@ func LoadBlobContext(ctx context.Context, root, format string, locked LockedBlob if len(locked.SHA256) != sha256.Size*2 { return domain.Blob{}, "", errors.New("locked blob has invalid SHA-256 length") } + // The relative path is built from an already-validated hex digest, so there + // is no caller-supplied component to sanitise, and the os.Root handle below + // refuses to traverse a symlink out of the workspace. Running the general + // path check as well would repeat that containment once per blob. relativeName := filepath.Join(".snailmail", "cas", "sha256", locked.SHA256[:2], locked.SHA256) - name, err := WorkspacePath(root, filepath.ToSlash(relativeName)) + resolvedRoot, err := ResolveWorkspaceRoot(root) if err != nil { - return domain.Blob{}, "", err + return domain.Blob{}, "", fmt.Errorf("%w: resolve workspace root: %v", blob.ErrUnavailable, err) } - rootHandle, err := os.OpenRoot(root) + name := filepath.Join(resolvedRoot, relativeName) + rootHandle, err := os.OpenRoot(resolvedRoot) if err != nil { return domain.Blob{}, "", fmt.Errorf("%w: open workspace root: %v", blob.ErrUnavailable, err) } @@ -262,14 +267,11 @@ func EnsureBlob(ctx context.Context, root, format string, locked LockedBlob, sto return domain.Blob{}, "", localErr } relativeName := filepath.Join(".snailmail", "cas", "sha256", locked.SHA256[:2], locked.SHA256) - name, err = WorkspacePath(root, filepath.ToSlash(relativeName)) - if err != nil { - return domain.Blob{}, "", err - } - resolvedRoot, err := filepath.EvalSymlinks(root) + resolvedRoot, err := ResolveWorkspaceRoot(root) if err != nil { return domain.Blob{}, "", err } + name = filepath.Join(resolvedRoot, relativeName) rootHandle, err := os.OpenRoot(resolvedRoot) if err != nil { return domain.Blob{}, "", err diff --git a/internal/state/cas_containment_test.go b/internal/state/cas_containment_test.go new file mode 100644 index 0000000..5cc5fee --- /dev/null +++ b/internal/state/cas_containment_test.go @@ -0,0 +1,66 @@ +package state + +import ( + "os" + "path/filepath" + "testing" + + "github.com/shellcell/snailmail/internal/testutil" +) + +// The blob path no longer runs the general workspace path check, so the os.Root +// handle is what keeps a CAS lookup inside the workspace. These hold that line. +func TestLoadBlobRejectsCASDirectorySymlinkedOutOfWorkspace(t *testing.T) { + root := t.TempDir() + outside := t.TempDir() + + source, err := testutil.WriteWheel(t.TempDir(), "snail-demo", "1.2.3", ">=3.9") + if err != nil { + t.Fatal(err) + } + blob, err := PutArtifact(root, "pypi", source) + if err != nil { + t.Fatal(err) + } + locked := ToLockedBlob(blob) + + // Move the whole shard outside the workspace and leave a symlink behind. + shard := filepath.Join(root, ".snailmail", "cas", "sha256", locked.SHA256[:2]) + relocated := filepath.Join(outside, "shard") + if err := os.Rename(shard, relocated); err != nil { + t.Fatal(err) + } + if err := os.Symlink(relocated, shard); err != nil { + t.Fatal(err) + } + if _, _, err := LoadBlob(root, "pypi", locked); err == nil { + t.Fatal("a CAS shard symlinked out of the workspace was accepted") + } +} + +func TestLoadBlobRejectsBlobThatIsItselfASymlink(t *testing.T) { + root := t.TempDir() + outside := t.TempDir() + + source, err := testutil.WriteWheel(t.TempDir(), "snail-demo", "1.2.3", ">=3.9") + if err != nil { + t.Fatal(err) + } + blob, err := PutArtifact(root, "pypi", source) + if err != nil { + t.Fatal(err) + } + locked := ToLockedBlob(blob) + + stored := filepath.Join(root, ".snailmail", "cas", "sha256", locked.SHA256[:2], locked.SHA256) + relocated := filepath.Join(outside, "object") + if err := os.Rename(stored, relocated); err != nil { + t.Fatal(err) + } + if err := os.Symlink(relocated, stored); err != nil { + t.Fatal(err) + } + if _, _, err := LoadBlob(root, "pypi", locked); err == nil { + t.Fatal("a CAS object that is a symlink was accepted") + } +} diff --git a/internal/state/store.go b/internal/state/store.go index 55f8cdd..298f723 100644 --- a/internal/state/store.go +++ b/internal/state/store.go @@ -17,6 +17,7 @@ import ( "regexp" "sort" "strings" + "sync" "time" "unicode" @@ -1192,11 +1193,40 @@ func ValidateArtifactOrigin(origin ArtifactOrigin) error { return nil } +// resolvedRoots caches the symlink resolution of a workspace root. A root +// cannot change while an operation holds the workspace lock, and resolving it +// is otherwise repeated for every path, including once per blob in a build. +var resolvedRoots struct { + sync.Mutex + byRoot map[string]string +} + +// ResolveWorkspaceRoot returns the workspace root with symlinks resolved. +func ResolveWorkspaceRoot(root string) (string, error) { + resolvedRoots.Lock() + cached, found := resolvedRoots.byRoot[root] + resolvedRoots.Unlock() + if found { + return cached, nil + } + resolved, err := filepath.EvalSymlinks(root) + if err != nil { + return "", err + } + resolvedRoots.Lock() + if resolvedRoots.byRoot == nil { + resolvedRoots.byRoot = make(map[string]string) + } + resolvedRoots.byRoot[root] = resolved + resolvedRoots.Unlock() + return resolved, nil +} + func WorkspacePath(root, relative string) (string, error) { if err := validateRelativePath(relative); err != nil { return "", err } - resolvedRoot, err := filepath.EvalSymlinks(root) + resolvedRoot, err := ResolveWorkspaceRoot(root) if err != nil { return "", err } From f44853b32254cc5678c5f96241f6e3bac0279027 Mon Sep 17 00:00:00 2001 From: Polina Simonenko Date: Tue, 28 Jul 2026 13:53:52 +0200 Subject: [PATCH 20/22] prefer a real static server for Helm verification --- internal/app/verify.go | 37 ++++++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/internal/app/verify.go b/internal/app/verify.go index 24544a0..c6a4c72 100644 --- a/internal/app/verify.go +++ b/internal/app/verify.go @@ -738,9 +738,40 @@ else fi HANDLER chmod 0700 /tmp/http-handler -nc -lk -s 127.0.0.1 -p 8879 -e /tmp/http-handler & -server=$! -trap 'kill "$server"' EXIT +# busybox httpd is a real static file server and is present in any +# busybox-based image. The nc handler stays as the fallback: -e and -k vary +# between nc implementations, which is why it is not the first choice. +server="" +if command -v busybox >/dev/null 2>&1 && busybox httpd --help >/dev/null 2>&1; then + busybox httpd -f -p 127.0.0.1:8879 -h /repo & + server=$! + sleep 1 + if ! kill -0 "$server" 2>/dev/null; then + server="" + fi +fi +if test -z "$server"; then + nc -lk -s 127.0.0.1 -p 8879 -e /tmp/http-handler & + server=$! +fi +trap 'kill "$server" 2>/dev/null' EXIT +# Fail with the image contract rather than an opaque Helm error when whichever +# server was chosen never comes up. +if command -v wget >/dev/null 2>&1; then + ready="" + for attempt in 1 2 3 4 5 6 7 8 9 10; do + if wget -q -O /dev/null http://127.0.0.1:8879/index.yaml 2>/dev/null; then + ready=yes + break + fi + sleep 1 + done + if test -z "$ready"; then + echo "verification image provides no usable static HTTP server;" >&2 + echo "it needs busybox httpd, or an nc supporting -l -k -s -p -e" >&2 + exit 1 + fi +fi helm repo add staged http://127.0.0.1:8879 helm repo update staged if test -n "$SNAILMAIL_CHART"; then From f92c11eea8d8841dee79a37e16a998d9e8d924d2 Mon Sep 17 00:00:00 2001 From: Polina Simonenko Date: Tue, 28 Jul 2026 13:59:03 +0200 Subject: [PATCH 21/22] document the platform and staging behaviour that changed --- README.md | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 089cfab..ca85e57 100644 --- a/README.md +++ b/README.md @@ -12,8 +12,9 @@ builds, structurally verifies, serves, and client-tests deterministic static PyPI, Debian, and Helm repositories. Git-backed workspaces also support local `init`, `setup`, `add`, `plan`, and `apply` workflows with immutable blobs, reviewable plans, publication ledgers, and per-repository managed release -switching. Replacement of an existing managed release is currently implemented -only on Linux. PyPI repositories can additionally stage and publish to +switching. Replacement of an existing managed release needs an atomic +directory-entry exchange, so it is implemented on Linux and macOS; creating a +first release is portable. PyPI repositories can additionally stage and publish to S3-compatible object storage with immutable releases, conditional root-index switching, checksum observation, conditional restore, and optional scoped private reads. Shared S3 blob storage, public GitHub Pages with a companion @@ -285,6 +286,11 @@ locks, ledgers, deployment receipts, and an optional current plan: go run ./cmd/snailmail render --output site ``` +`plan` and `apply` build and stage under `.snailmail/stage` in the workspace +rather than in `TMPDIR`, so artifacts hard-link from the local CAS instead of +being copied and large trees are never held in a `tmpfs`. Set an absolute +`SNAILMAIL_STAGE_DIR` to stage elsewhere. + The AWS SDK is the largest single component of the binary. Builds that only target local directories or GitHub Pages can exclude it: From 49906948c29ba6b5f71aebe3aa31efead052a623 Mon Sep 17 00:00:00 2001 From: Polina Simonenko Date: Tue, 28 Jul 2026 14:06:44 +0200 Subject: [PATCH 22/22] fixed ci tests Signed-off-by: Polina Simonenko --- adapters/host/s3/parallel_test.go | 46 +++++++++++++++++++------------ internal/state/git.go | 8 +++++- internal/state/gitlock_test.go | 33 +++++++++++++++++++++- 3 files changed, 67 insertions(+), 20 deletions(-) diff --git a/adapters/host/s3/parallel_test.go b/adapters/host/s3/parallel_test.go index a99e681..7bb68a3 100644 --- a/adapters/host/s3/parallel_test.go +++ b/adapters/host/s3/parallel_test.go @@ -7,6 +7,7 @@ import ( "sync" "sync/atomic" "testing" + "time" ) func TestForEachObjectRunsEveryIndexOnce(t *testing.T) { @@ -31,28 +32,37 @@ func TestForEachObjectRunsEveryIndexOnce(t *testing.T) { } } -func TestForEachObjectActuallyOverlaps(t *testing.T) { - var inFlight, peak atomic.Int64 - if err := forEachObject(context.Background(), 64, func(_ context.Context, _ int) error { - current := inFlight.Add(1) - for { - previous := peak.Load() - if current <= previous || peak.CompareAndSwap(previous, current) { - break - } +// The pool must have several requests in flight at once. This waits for a +// rendezvous rather than sampling a counter: sampling measures parallelism, +// which a single-CPU machine cannot show even when the pool is working +// correctly, whereas independent progress is what the pool actually promises. +func TestForEachObjectOverlapsRequests(t *testing.T) { + const overlapping = 2 + arrived := make(chan struct{}, overlapping) + release := make(chan struct{}) + go func() { + for range overlapping { + <-arrived } - // Hold the slot so other workers have a chance to overlap. - for i := 0; i < 20000; i++ { - _ = i + close(release) + }() + err := forEachObject(context.Background(), 8, func(_ context.Context, index int) error { + if index >= overlapping { + return nil } - inFlight.Add(-1) - return nil - }); err != nil { + arrived <- struct{}{} + select { + case <-release: + return nil + case <-time.After(30 * time.Second): + // Only reachable if the pool ran these one after another, since the + // release waits for both to have started. + return fmt.Errorf("request %d ran alone; the pool did not overlap requests", index) + } + }) + if err != nil { t.Fatal(err) } - if peak.Load() < 2 { - t.Fatalf("peak concurrency was %d; work did not overlap", peak.Load()) - } } // Whichever request loses the race, the reported failure must be the same one, diff --git a/internal/state/git.go b/internal/state/git.go index 0cbe645..f0abaa1 100644 --- a/internal/state/git.go +++ b/internal/state/git.go @@ -1078,7 +1078,7 @@ func describeGitLockConflict(blocked string, names []string, cause error) error return fmt.Errorf("%s is held by another Git process", blocked) } details := strings.ReplaceAll(strings.TrimSpace(string(content)), "\n", " ") - if owner, found := gitLockOwnerProcess(content); found && processExists(owner) { + if owner, found := gitLockOwnerProcess(content); found && gitLockOwnerIsRunning(owner) { return fmt.Errorf("%s is held by a running snailmail process (%s)", blocked, details) } return fmt.Errorf("%s was left behind by a snailmail run that did not finish (%s); "+ @@ -1096,6 +1096,12 @@ func gitLockOwnerProcess(content []byte) (int, bool) { return 0, false } +// gitLockOwnerIsRunning is a variable so tests can decide the answer instead of +// depending on which process ids happen to exist. Signalling a live process +// also answers differently for root and for an ordinary user, so no literal pid +// gives the same result everywhere. +var gitLockOwnerIsRunning = processExists + // processExists reports whether a process id is live. A recycled id can make // this a false positive, which is why the caller only uses it to soften the // wording rather than to remove anything. diff --git a/internal/state/gitlock_test.go b/internal/state/gitlock_test.go index 2e646a4..c70f85e 100644 --- a/internal/state/gitlock_test.go +++ b/internal/state/gitlock_test.go @@ -7,6 +7,36 @@ import ( "testing" ) +func withGitLockOwnerRunning(t *testing.T, running func(int) bool) { + t.Helper() + previous := gitLockOwnerIsRunning + gitLockOwnerIsRunning = running + t.Cleanup(func() { gitLockOwnerIsRunning = previous }) +} + +// A lock whose owner is still alive is reported as busy, not as recoverable +// wreckage, so the message must not invite anyone to delete it. +func TestGitLockConflictReportsRunningOwnerAsBusy(t *testing.T) { + directory := t.TempDir() + blocked := filepath.Join(directory, "index.lock") + withGitLockOwnerRunning(t, func(int) bool { return true }) + held := gitLockOwnerPrefix + "\npid=4242\nsince=2026-01-01T00:00:00Z\n" + if err := os.WriteFile(blocked, []byte(held), 0o600); err != nil { + t.Fatal(err) + } + _, err := acquireGitLock(blocked) + if err == nil { + t.Fatal("expected acquiring an existing lock to fail") + } + message := describeGitLockConflict(blocked, []string{blocked}, err).Error() + if !strings.Contains(message, "running snailmail process") { + t.Fatalf("message %q does not report a live owner", message) + } + if strings.Contains(message, "remove") { + t.Fatalf("a lock a live process holds must not be recommended for removal: %q", message) + } +} + func TestGitLockRecordsItsOwner(t *testing.T) { name := filepath.Join(t.TempDir(), "index.lock") release, err := acquireGitLock(name) @@ -35,7 +65,8 @@ func TestGitLockConflictExplainsAbandonedLock(t *testing.T) { blocked := filepath.Join(directory, "index.lock") names := []string{blocked, filepath.Join(directory, "HEAD.lock")} - abandoned := gitLockOwnerPrefix + "\npid=1\nsince=2026-01-01T00:00:00Z\n" + withGitLockOwnerRunning(t, func(int) bool { return false }) + abandoned := gitLockOwnerPrefix + "\npid=4242\nsince=2026-01-01T00:00:00Z\n" if err := os.WriteFile(blocked, []byte(abandoned), 0o600); err != nil { t.Fatal(err) }