diff --git a/docs/structured-output.md b/docs/structured-output.md index 45609971..77279621 100644 --- a/docs/structured-output.md +++ b/docs/structured-output.md @@ -254,6 +254,7 @@ Codes: `EMULATOR_NOT_CONFIGURED` (no AWS container configured), `EMULATOR_NOT_RU "error": null } ``` +When the installed version is current but its bundled-extension set is incomplete, `updateAvailable` is `true` with `currentVersion` equal to `latestVersion` (modulo the `v` prefix) and the check shape additionally carries `"repairBundled": true`. The key is absent otherwise, and is what distinguishes a same-version repair from an ordinary upgrade. It belongs to the check shape only: an applied update reports the shape below regardless of what triggered it. ```json { "schemaVersion": 1, diff --git a/internal/output/envelope_sink.go b/internal/output/envelope_sink.go index b669a22d..0edadc16 100644 --- a/internal/output/envelope_sink.go +++ b/internal/output/envelope_sink.go @@ -56,13 +56,21 @@ func (s *EnvelopeSink) Emit(event Event) { s.data["currentVersion"] = e.CurrentVersion s.data["latestVersion"] = e.LatestVersion s.data["updateAvailable"] = e.Available + // Only present when true, so the pre-existing shapes are unchanged. It + // is what lets a JSON consumer tell a same-version bundled-set repair + // from an ordinary upgrade; the version fields alone cannot (they + // differ only by the "v" prefix in the repair case). + if e.RepairBundled { + s.data["repairBundled"] = true + } case UpdateAppliedEvent: // An UpdateCheckedEvent always precedes this on the apply path (Check // fires it unconditionally now, for the plain-text "Update available" - // line), so clear the keys it set rather than leaving stale - // latestVersion/updateAvailable alongside the applied-update shape. + // line), so clear every key it set rather than leaving stale check + // fields alongside the applied-update shape. delete(s.data, "latestVersion") delete(s.data, "updateAvailable") + delete(s.data, "repairBundled") s.data["currentVersion"] = e.CurrentVersion s.data["updatedVersion"] = e.UpdatedVersion s.data["updated"] = true diff --git a/internal/output/envelope_sink_test.go b/internal/output/envelope_sink_test.go index 6f52c17b..251cbcc6 100644 --- a/internal/output/envelope_sink_test.go +++ b/internal/output/envelope_sink_test.go @@ -87,6 +87,26 @@ func TestEnvelopeSink_UpdateCheckedEvent(t *testing.T) { if _, hasApplied := data["updated"]; hasApplied { t.Fatalf("did not expect an 'updated' key from UpdateCheckedEvent: %+v", data) } + if _, hasRepair := data["repairBundled"]; hasRepair { + t.Fatalf("repairBundled must be absent on an ordinary check: %+v", data) + } +} + +// TestEnvelopeSink_UpdateCheckedRepairBundled covers the same-version +// bundled-set repair: the version fields differ only by the "v" prefix, so +// the repairBundled key is the only way a JSON consumer can tell a repair +// from an ordinary upgrade. +func TestEnvelopeSink_UpdateCheckedRepairBundled(t *testing.T) { + t.Parallel() + + sink := NewEnvelopeSink(FormatJSON) + sink.Emit(UpdateCheckedEvent{CurrentVersion: "2.3.0", LatestVersion: "v2.3.0", Available: true, RepairBundled: true}) + + envelope := sink.Result("update", nil) + data := envelope.Data.(map[string]any) + if data["updateAvailable"] != true || data["repairBundled"] != true { + t.Fatalf("unexpected data: %+v", data) + } } // TestEnvelopeSink_UpdateCheckedEnvelopeJSON pins the full serialized @@ -337,3 +357,23 @@ func TestSlugify(t *testing.T) { } } } + +// TestEnvelopeSink_UpdateAppliedClearsRepairBundled: the applied-update shape +// documents no repairBundled key, so the cleanup that already scrubs +// latestVersion and updateAvailable must scrub it too. +func TestEnvelopeSink_UpdateAppliedClearsRepairBundled(t *testing.T) { + t.Parallel() + + sink := NewEnvelopeSink(FormatJSON) + sink.Emit(UpdateCheckedEvent{CurrentVersion: "2.3.0", LatestVersion: "v2.3.0", Available: true, RepairBundled: true}) + sink.Emit(UpdateAppliedEvent{CurrentVersion: "2.3.0", UpdatedVersion: "v2.3.0", Method: "binary"}) + + envelope := sink.Result("update", nil) + data := envelope.Data.(map[string]any) + if _, has := data["repairBundled"]; has { + t.Fatalf("repairBundled must not leak into the applied-update shape: %+v", data) + } + if data["updated"] != true { + t.Fatalf("expected updated: true, got %+v", data) + } +} diff --git a/internal/output/events.go b/internal/output/events.go index 1ca11365..ac4f5671 100644 --- a/internal/output/events.go +++ b/internal/output/events.go @@ -206,15 +206,21 @@ type EmulatorStatusEvent struct { } // UpdateCheckedEvent reports the result of an update check. It always fires -// once per Check call — DevBuild, then Available, discriminate which of the -// three possible outcomes (dev build skipped the check / already up to date / -// an update is available) the formatter should render. LatestVersion is empty -// when DevBuild is true (the check never ran). +// once per Check call. DevBuild, then RepairBundled, then Available, +// discriminate which of the four possible outcomes (dev build skipped the +// check / already up to date / the installed set is incomplete and is being +// repaired / an update is available) the formatter should render. +// LatestVersion is empty when DevBuild is true (the check never ran). type UpdateCheckedEvent struct { CurrentVersion string LatestVersion string Available bool DevBuild bool + // RepairBundled reports that the check found the installed version current + // but its bundled-extension set incomplete, so the update reinstalls the + // same version rather than reporting "already up to date". Implies + // Available, and CurrentVersion equals LatestVersion. + RepairBundled bool } // UpdateAppliedEvent reports that an update was downloaded and installed. diff --git a/internal/output/plain_format.go b/internal/output/plain_format.go index 020103b4..abd17176 100644 --- a/internal/output/plain_format.go +++ b/internal/output/plain_format.go @@ -223,6 +223,11 @@ func formatUpdateChecked(e UpdateCheckedEvent) string { switch { case e.DevBuild: return "> Note: Running a development build, skipping update check" + case e.RepairBundled: + // States only the finding: this line also renders under --check, where + // nothing is reinstalled. The apply path narrates the reinstall itself + // (see update.Update). + return fmt.Sprintf("Bundled extensions are missing from this install (version %s)", e.CurrentVersion) case !e.Available: return fmt.Sprintf("> Note: Already up to date (%s)", e.CurrentVersion) default: diff --git a/internal/output/plain_format_test.go b/internal/output/plain_format_test.go index 53c326db..31590e05 100644 --- a/internal/output/plain_format_test.go +++ b/internal/output/plain_format_test.go @@ -223,6 +223,12 @@ func TestFormatEventLine(t *testing.T) { want: "Update available: 2.2.1 → 2.3.0", wantOK: true, }, + { + name: "update checked event repairing bundled set", + event: UpdateCheckedEvent{CurrentVersion: "2.3.0", LatestVersion: "v2.3.0", Available: true, RepairBundled: true}, + want: "Bundled extensions are missing from this install (version 2.3.0)", + wantOK: true, + }, { name: "update applied event", event: UpdateAppliedEvent{CurrentVersion: "2.2.1", UpdatedVersion: "2.3.0", Method: "homebrew"}, diff --git a/internal/update/bundled.go b/internal/update/bundled.go new file mode 100644 index 00000000..bdb06d3c --- /dev/null +++ b/internal/update/bundled.go @@ -0,0 +1,91 @@ +package update + +import ( + "errors" + "io/fs" + "os" + "path/filepath" + goruntime "runtime" + + "github.com/localstack/lstk/internal/extension" + "github.com/localstack/lstk/internal/log" + "github.com/localstack/lstk/internal/version" +) + +// missingBundledMembers reports which members of the bundled set this release +// ships are absent from the install directory, so `lstk update` can repair an +// incomplete install instead of short-circuiting on the version alone. +// +// It exists for one specific situation. A user crossing the transition on the +// binary channel is updated by their *old* lstk, which installs only the lstk +// binary and ignores the archive's other members, leaving a current binary +// with no bundled extensions. They cannot fix that by updating again, because +// applyUpdate always jumps straight to the newest release: they are already on +// it, so a version comparison reports "already up to date" until another +// release ships, potentially a week later. +// +// The directory probed is extension.BundledDir, by definition the directory +// the resolver reads bundled extensions from, so the completeness check and +// resolution can never look in different places. +// +// It returns nil, leaving the version comparison as the sole trigger exactly +// as before, in two cases that matter: +// +// - The release ships no bundle (version.BundledSet is empty). Every +// pre-bundling release, and any rollback to one, is therefore unaffected. +// - lstk was installed by Homebrew or npm, where the package manager replaces +// the whole package and so owns set completeness itself. Repairing there +// would mean shelling out to the package manager for a version it has +// already installed. +func missingBundledMembers() []string { + expected := version.BundledSet() + if len(expected) == 0 { + return nil + } + if DetectInstallMethod().Method != InstallBinary { + return nil + } + dir := extension.BundledDir(log.Nop()) + if dir == "" { + return nil + } + return missingSetMembers(dir, expected) +} + +// missingSetMembers returns the expected names that are absent from dir, in +// the order they were expected. "Absent" means missing or unusable: a name +// that stats but cannot run (a directory, or a binary without its exec bit) +// leaves the user exactly as stranded as no file at all, so it triggers the +// same repair. A stat failure that is not absence (a permission or I/O error) +// counts as present instead, because treating it as missing would re-download +// the whole release on every run over a transient error. +func missingSetMembers(dir string, expected []string) []string { + var missing []string + for _, name := range expected { + info, err := os.Stat(filepath.Join(dir, name)) + switch { + case errors.Is(err, fs.ErrNotExist): + missing = append(missing, name) + case err != nil: + continue + case !usableSetMember(info, name): + missing = append(missing, name) + } + } + return missing +} + +// usableSetMember mirrors what extension resolution will accept (the +// resolver's side of the rule is isExecutableFile in internal/extension): a +// regular file, with an exec bit on Unix unless it is the descriptions file. +// On Windows executability is carried by the name, which the stamped set +// already includes. +func usableSetMember(info os.FileInfo, name string) bool { + if !info.Mode().IsRegular() { + return false + } + if name == descriptionsFileName || goruntime.GOOS == "windows" { + return true + } + return info.Mode().Perm()&0o111 != 0 +} diff --git a/internal/update/bundled_test.go b/internal/update/bundled_test.go new file mode 100644 index 00000000..2f4f5920 --- /dev/null +++ b/internal/update/bundled_test.go @@ -0,0 +1,230 @@ +package update + +import ( + "context" + "net/http/httptest" + "os" + "path/filepath" + goruntime "runtime" + "testing" + + "github.com/localstack/lstk/internal/output" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMissingSetMembers(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + present []string + expected []string + want []string + }{ + { + name: "release ships no bundle", + present: []string{"lstk"}, + expected: nil, + want: nil, + }, + { + name: "complete set", + present: []string{"lstk", "bundled-extensions", "lstk-extensions.toml"}, + expected: []string{"bundled-extensions", "lstk-extensions.toml"}, + want: nil, + }, + { + // The transition case: a pre-bundling updater installed only lstk. + name: "nothing but lstk after crossing the transition", + present: []string{"lstk"}, + expected: []string{"bundled-extensions", "lstk-extensions.toml"}, + want: []string{"bundled-extensions", "lstk-extensions.toml"}, + }, + { + name: "one member missing", + present: []string{"lstk", "bundled-extensions"}, + expected: []string{"bundled-extensions", "lstk-extensions.toml"}, + want: []string{"lstk-extensions.toml"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + for _, name := range tc.present { + require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte("x"), 0o755)) + } + assert.Equal(t, tc.want, missingSetMembers(dir, tc.expected)) + }) + } +} + +// newGitHubServerWithCleanup reuses the package's one mock GitHub +// latest-release server (newTestGitHubServer in notify_test.go) and ties its +// shutdown to the test, so this file cannot grow a drifting second mock. +func newGitHubServerWithCleanup(t *testing.T, tag string) *httptest.Server { + t.Helper() + srv := newTestGitHubServer(t, tag) + t.Cleanup(srv.Close) + return srv +} + +// formattedLine renders an event the way the plain sink would. +func formattedLine(t *testing.T, event output.Event) string { + t.Helper() + line, ok := output.FormatEventLine(event) + require.True(t, ok, "event should have a plain-text rendering") + return line +} + +// checkedEvent returns the single UpdateCheckedEvent a check emitted. +func checkedEvent(t *testing.T, events []output.Event) output.UpdateCheckedEvent { + t.Helper() + for _, e := range events { + if checked, ok := e.(output.UpdateCheckedEvent); ok { + return checked + } + } + t.Fatal("no UpdateCheckedEvent was emitted") + return output.UpdateCheckedEvent{} +} + +// TestCheckRepairsIncompleteSetAtCurrentVersion is the repair half of the +// transition: the binary is already the latest version, so the version +// comparison alone would report "already up to date" and leave the user +// without their bundled extensions until another release ships. +func TestCheckRepairsIncompleteSetAtCurrentVersion(t *testing.T) { + srv := newGitHubServerWithCleanup(t, "v1.2.3") + t.Setenv(githubAPIEndpointEnv, srv.URL) + + var events []output.Event + sink := output.SinkFunc(func(e output.Event) { events = append(events, e) }) + latest, available, repair, err := checkWithVersion(context.Background(), sink, "", "1.2.3", func() []string { + return []string{"bundled-extensions", "lstk-extensions.toml"} + }) + + require.NoError(t, err) + assert.Equal(t, "v1.2.3", latest) + assert.True(t, available, "an incomplete set must not short-circuit on the version") + assert.True(t, repair, "Check must hand the repair fact to its caller, not have it re-derived") + + event := checkedEvent(t, events) + assert.True(t, event.Available) + assert.True(t, event.RepairBundled, "the check must say it is repairing, not that an upgrade is available") + assert.NotContains(t, formattedLine(t, event), "Already up to date") +} + +// TestCheckReportsUpToDateWhenSetIsComplete keeps the ordinary path as cheap +// as it was: same version, complete set, nothing to do and nothing downloaded. +func TestCheckReportsUpToDateWhenSetIsComplete(t *testing.T) { + srv := newGitHubServerWithCleanup(t, "v1.2.3") + t.Setenv(githubAPIEndpointEnv, srv.URL) + + var events []output.Event + sink := output.SinkFunc(func(e output.Event) { events = append(events, e) }) + _, available, repair, err := checkWithVersion(context.Background(), sink, "", "1.2.3", func() []string { return nil }) + + require.NoError(t, err) + assert.False(t, available) + assert.False(t, repair) + + event := checkedEvent(t, events) + assert.False(t, event.Available) + assert.False(t, event.RepairBundled) + assert.Contains(t, formattedLine(t, event), "Already up to date") +} + +// TestCheckPrefersVersionUpgradeOverRepair keeps the two reasons distinct: when +// a newer release exists, this is an ordinary upgrade even if the set is also +// incomplete, and the installed set is repaired by that upgrade anyway. +func TestCheckPrefersVersionUpgradeOverRepair(t *testing.T) { + srv := newGitHubServerWithCleanup(t, "v2.0.0") + t.Setenv(githubAPIEndpointEnv, srv.URL) + + var events []output.Event + sink := output.SinkFunc(func(e output.Event) { events = append(events, e) }) + _, available, repair, err := checkWithVersion(context.Background(), sink, "", "1.2.3", func() []string { + return []string{"bundled-extensions"} + }) + + require.NoError(t, err) + assert.True(t, available) + assert.False(t, repair, "a version upgrade is not a repair even when members are also missing") + + event := checkedEvent(t, events) + assert.False(t, event.RepairBundled) + assert.Contains(t, formattedLine(t, event), "Update available: 1.2.3 → v2.0.0") +} + +// TestCheckSkipsRepairForDevBuilds keeps a dev build out of the network path +// entirely, as before. +func TestCheckSkipsRepairForDevBuilds(t *testing.T) { + var events []output.Event + sink := output.SinkFunc(func(e output.Event) { events = append(events, e) }) + _, available, repair, err := checkWithVersion(context.Background(), sink, "", "dev", func() []string { + return []string{"bundled-extensions"} + }) + + require.NoError(t, err) + assert.False(t, available) + assert.False(t, repair) + assert.True(t, checkedEvent(t, events).DevBuild) +} + +// TestMissingSetMembersRequiresUsableFiles: "present" must mean what the +// extension resolver means by resolvable. A file that exists but cannot run +// leaves the user exactly as stranded as an absent one. +func TestMissingSetMembersRequiresUsableFiles(t *testing.T) { + t.Parallel() + if goruntime.GOOS == "windows" { + t.Skip("no exec bits on Windows") + } + + t.Run("non-executable binary counts as missing", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "bundled-extensions"), []byte("x"), 0o644)) + got := missingSetMembers(dir, []string{"bundled-extensions"}) + assert.Equal(t, []string{"bundled-extensions"}, got) + }) + + t.Run("directory squatting on the name counts as missing", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, "bundled-extensions"), 0o755)) + got := missingSetMembers(dir, []string{"bundled-extensions"}) + assert.Equal(t, []string{"bundled-extensions"}, got) + }) + + t.Run("descriptions file needs no exec bit", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "lstk-extensions.toml"), []byte("x"), 0o644)) + got := missingSetMembers(dir, []string{"lstk-extensions.toml"}) + assert.Nil(t, got) + }) +} + +// TestMissingSetMembersIgnoresUnreadableDir: a stat failure that is not +// "does not exist" must not count as missing. Treating a transient permission +// or I/O error as absence would trigger a pointless full re-download on every +// run. +func TestMissingSetMembersIgnoresUnreadableDir(t *testing.T) { + if goruntime.GOOS == "windows" { + t.Skip("directory permission bits do not gate stat on Windows") + } + if os.Geteuid() == 0 { + t.Skip("root bypasses permission checks") + } + parent := t.TempDir() + dir := filepath.Join(parent, "inner") + require.NoError(t, os.MkdirAll(dir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "bundled-extensions"), []byte("x"), 0o755)) + require.NoError(t, os.Chmod(dir, 0o000)) + t.Cleanup(func() { _ = os.Chmod(dir, 0o755) }) + + got := missingSetMembers(dir, []string{"bundled-extensions"}) + assert.Nil(t, got, "an unreadable install dir is not evidence of a missing member") +} diff --git a/internal/update/extract.go b/internal/update/extract.go index f46d052d..da22d6c6 100644 --- a/internal/update/extract.go +++ b/internal/update/extract.go @@ -10,9 +10,122 @@ import ( "path/filepath" goruntime "runtime" "strings" + + "github.com/localstack/lstk/internal/extension" ) +// stagingSuffix marks a set member that has been copied into the install +// directory but not yet committed under its final name. Staging inside the +// destination directory (rather than in the extraction temp dir) is what makes +// every commit an intra-directory rename: same filesystem, so it is atomic and +// cannot fail with EXDEV. The cross-device case that used to need a copy +// fallback is handled by construction, because the only cross-filesystem hop +// (temp dir to install dir) is always a copy. +const stagingSuffix = ".lstk-new" + +// descriptionsFileName is the bundled descriptions file an archive ships +// alongside the extension binaries. It is aliased from the extension package so +// the updater and the runtime resolver can never disagree on its name. +const descriptionsFileName = extension.DescriptionsFileName + +// bundledBinaryBaseName is the single multi-call binary that provides every +// bundled extension: one program that dispatches on the command it is asked +// for, rather than one binary per extension. It must be discovered by exact +// name, because it is the one member of the set that does not match "lstk-*". +// +// TODO(dpx-692): the extension-bundling branch exports this same name as +// extension.BundledBinaryName for the runtime resolver. Once that branch +// lands, alias this constant from there the way descriptionsFileName already +// is, so the updater and the resolver can never disagree on the name. +const bundledBinaryBaseName = "bundled-extensions" + +// exeName returns base with the platform executable suffix appended on +// Windows. Every construction of a member file name goes through this, so the +// suffix rule cannot drift between discovery, staging and the tests. +func exeName(base, goos string) string { + if goos == "windows" { + return base + ".exe" + } + return base +} + +// bundledBinaryName is the archive-root name of the multi-call binary on the +// given platform. +func bundledBinaryName(goos string) string { + return exeName(bundledBinaryBaseName, goos) +} + +// updateMember is one file of the version-matched set an update installs: the +// lstk binary, a bundled extension binary, or the descriptions file. +type updateMember struct { + src string // path inside the extracted archive + dest string // final path in the install directory + mode os.FileMode // permissions to install with +} + +// staging is the temporary sibling this member is copied to before commit. +func (m updateMember) staging() string { return m.dest + stagingSuffix } + +// commit renames the staged copy over the member's final name. +func (m updateMember) commit(goos string) error { + // On Windows a running executable cannot be replaced but can be renamed, + // so every existing member is moved aside first. That obviously covers the + // lstk.exe executing this update, but also a bundled extension the user + // happens to be running in another terminal while the update commits. The + // ".old" file is left behind on success (the running lstk.exe cannot + // delete itself) and is cleaned up by the next update's commit. + movedAside := "" + if goos == "windows" { + oldPath := m.dest + ".old" + // Clean up a leftover from a previous update; ignore if absent. + if err := os.Remove(oldPath); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("cannot remove old binary %s: %w", oldPath, err) + } + switch err := os.Rename(m.dest, oldPath); { + case err == nil: + movedAside = oldPath + case os.IsNotExist(err): + // A member the install did not have yet; nothing to move. + default: + return fmt.Errorf("cannot move %s aside: %w", filepath.Base(m.dest), err) + } + } + if err := os.Rename(m.staging(), m.dest); err != nil { + if movedAside != "" { + // The old file was already moved aside, so failing here would leave + // nothing under the real name. For lstk itself that would mean no + // binary left to re-run the update with. Move it back so the user + // keeps the previous version. + if rerr := os.Rename(movedAside, m.dest); rerr != nil { + return fmt.Errorf("%w (restoring the previous file also failed: %v; rename %s back to %s by hand to recover)", + err, rerr, movedAside, m.dest) + } + } + return err + } + return nil +} + +// extractAndReplace extracts the downloaded release archive and installs every +// member of the set it carries (the lstk binary, the multi-call +// "bundled-extensions" binary, any "lstk-*" extension binaries, and the +// descriptions file) as one unit, using stage-then-commit. +// +// The set is whatever the archive contains: an archive carrying only lstk (a +// pre-bundling release, or a rollback to one) is a valid set of size one and is +// installed exactly as the pre-bundling updater did. When an archive does carry +// extensions they are not optional: any member that fails to stage or commit +// fails the whole update, naming the member, rather than reporting success with +// a partial set. func extractAndReplace(archivePath, exePath, format string) error { + return replaceSet(archivePath, exePath, format, goruntime.GOOS) +} + +// replaceSet is extractAndReplace with the target platform as a parameter, so +// the Windows naming rules and the move-aside commit can be exercised from a +// test on any host. Unit tests run on Linux only in CI, and this is the update +// path: the one thing a bad release cannot ship a fix for. +func replaceSet(archivePath, exePath, format, goos string) error { dir, err := os.MkdirTemp("", "lstk-extract-*") if err != nil { return err @@ -30,40 +143,205 @@ func extractAndReplace(archivePath, exePath, format string) error { } } - binaryName := "lstk" - if goruntime.GOOS == "windows" { - binaryName = "lstk.exe" + members, err := discoverMembers(dir, exePath, goos) + if err != nil { + return err + } + + // A previous update that died between staging and commit leaves staging + // files behind. Removing them first is what makes re-running `lstk update` + // a clean repair instead of a resume of unknown state. + if err := removeStagingFiles(filepath.Dir(exePath)); err != nil { + return err + } + + if err := stageMembers(members); err != nil { + return err } + return commitMembers(members, goos) +} + +// discoverMembers builds the set to install from the extracted archive root: +// the multi-call bundled binary, every executable "lstk-*" file, the +// descriptions file, and the lstk binary. The "lstk-*" case covers an archive +// that ships extensions as standalone binaries; a bundle built the current way +// contributes none, because it ships one multi-call binary instead. +// +// The returned order is the commit order. The only ordering that matters is +// that the lstk binary goes last: committing lstk last is what keeps a failed +// commit safe, because any failure before that final rename leaves the user +// with a working lstk to re-run the update with. +func discoverMembers(extractDir, exePath, goos string) ([]updateMember, error) { + binaryName := exeName("lstk", goos) - newBinary := filepath.Join(dir, binaryName) + newBinary := filepath.Join(extractDir, binaryName) if _, err := os.Stat(newBinary); err != nil { - return fmt.Errorf("binary not found in archive: %w", err) + return nil, fmt.Errorf("binary not found in archive: %w", err) } - info, err := os.Stat(exePath) + exeInfo, err := os.Stat(exePath) if err != nil { - return err + return nil, err } - // On Windows, a running executable cannot be overwritten but can be renamed. - // Move it out of the way first so we can place the new binary at the original path. - if goruntime.GOOS == "windows" { - oldPath := exePath + ".old" - // Clean up leftover from a previous update; ignore error if it doesn't exist. - if err := os.Remove(oldPath); err != nil && !os.IsNotExist(err) { - return fmt.Errorf("cannot remove old binary %s: %w", oldPath, err) + destDir := filepath.Dir(exePath) + entries, err := os.ReadDir(extractDir) + if err != nil { + return nil, err + } + + // os.ReadDir sorts by filename, so the member order is deterministic. + var members []updateMember + for _, entry := range entries { + name := entry.Name() + if name == binaryName { + continue } - if err := os.Rename(exePath, oldPath); err != nil { - return fmt.Errorf("cannot move running binary: %w", err) + info, err := entry.Info() + if err != nil { + return nil, err + } + switch { + case name == bundledBinaryName(goos): + members = append(members, updateMember{ + src: filepath.Join(extractDir, name), + dest: filepath.Join(destDir, name), + mode: 0o755, + }) + case name == descriptionsFileName: + members = append(members, updateMember{ + src: filepath.Join(extractDir, name), + dest: filepath.Join(destDir, name), + mode: 0o644, + }) + case isExtensionEntry(name, info, goos): + members = append(members, updateMember{ + src: filepath.Join(extractDir, name), + dest: filepath.Join(destDir, name), + mode: 0o755, + }) } } - if err := os.Rename(newBinary, exePath); err != nil { - // Cross-device rename: fall back to copy - return copyFile(newBinary, exePath, info.Mode()) + // The destination is exePath rather than destDir/binaryName: the user may + // have installed the binary under a different name, and the pre-bundling + // updater replaced whatever it was running as. The full mode is preserved + // for the same reason, special bits included: an lstk installed setgid + // must still be setgid after the update (os.Chmod applies the + // setuid/setgid/sticky bits as well as the permissions). + return append(members, updateMember{ + src: newBinary, + dest: exePath, + mode: exeInfo.Mode(), + }), nil +} + +// isExtensionEntry reports whether an archive-root entry is a bundled +// extension binary. The descriptions file is excluded by the caller before this +// runs, since it shares the "lstk-" prefix. +// +// On Windows this is deliberately narrower than runtime resolution: the +// resolver accepts the whole PATHEXT set (see scanDir and windowsExts in +// internal/extension/resolve.go), while an installer must not let the user's +// PATHEXT decide what a release archive installs, so only ".exe" is accepted +// here. A release shipping any other launcher shape must widen both sides +// together. +func isExtensionEntry(name string, info os.FileInfo, goos string) bool { + if !strings.HasPrefix(name, extension.NamePrefix) || !info.Mode().IsRegular() { + return false } + if goos == "windows" { + // Windows archives carry no execute bit, so the ".exe" suffix (how the + // release names bundled extension binaries, and what resolution looks + // for) is what identifies one. Without this check any lstk-*.txt + // shipped at the archive root would install as an extension. + return strings.EqualFold(filepath.Ext(name), ".exe") + } + return info.Mode().Perm()&0o111 != 0 +} - return os.Chmod(exePath, info.Mode()) +// removeStagingFiles deletes staging files left in dir by an interrupted +// update. Only regular files are removed: the updater only ever creates regular +// files there, so anything else carrying the suffix belongs to the user and +// must not be deleted (stageMembers then refuses to write through it). +// +// The directory is listed and matched by literal suffix, never by pattern: +// the install path is user data, and a glob would misread metacharacters in +// it (an unmatched "[" fails outright, a matched pair matches nothing and +// silently skips the cleanup). +func removeStagingFiles(dir string) error { + entries, err := os.ReadDir(dir) + if err != nil { + return err + } + for _, entry := range entries { + if !strings.HasSuffix(entry.Name(), stagingSuffix) || !entry.Type().IsRegular() { + continue + } + path := filepath.Join(dir, entry.Name()) + if err := os.Remove(path); err != nil { + return fmt.Errorf("cannot remove leftover staging file %s: %w", path, err) + } + } + return nil +} + +// stageMembers copies every member next to its destination under the staging +// name. A failure removes what was staged so far and leaves the installation +// completely untouched, since nothing has been committed at this point. +// +// Anything already sitting at a staging path is refused rather than written +// over. The cleanup pass has just removed the updater's own leftovers, so a +// regular file appearing here means another update is running, and a +// non-regular one is the user's (writing through a symlink would destroy +// whatever it points at, and the commit would then install the symlink itself +// as the member). Refusing is also what keeps two concurrent updates from +// committing each other's half-written copies. +func stageMembers(members []updateMember) error { + staged := make([]string, 0, len(members)) + unstage := func() { + for _, path := range staged { + _ = os.Remove(path) + } + } + + for _, m := range members { + path := m.staging() + if info, err := os.Lstat(path); err == nil { + unstage() + if info.Mode().IsRegular() { + return fmt.Errorf("cannot stage %s: %s already exists; is another lstk update running?", + filepath.Base(m.dest), path) + } + return fmt.Errorf("cannot stage %s: %s exists and is not a regular file; move it out of the way and re-run lstk update", + filepath.Base(m.dest), path) + } + if err := copyFile(m.src, path, m.mode); err != nil { + _ = os.Remove(path) + unstage() + return fmt.Errorf("cannot stage %s in %s: %w (the update needs write permission in this directory)", + filepath.Base(m.dest), filepath.Dir(m.dest), err) + } + staged = append(staged, path) + } + return nil +} + +// commitMembers renames each staged file over its final name, in the order +// discoverMembers returned (lstk last). It stops at the first failure and +// names the member that failed. Staging files for members not yet committed +// are left for the next run to clean up, since removing them could fail for +// the same reason the rename did and mask the real error. Members committed +// before the failure stay at the new version; that skew is benign by contract +// (the extension API version only changes on breaking releases) and is +// resolved by re-running the update. +func commitMembers(members []updateMember, goos string) error { + for _, m := range members { + if err := m.commit(goos); err != nil { + return fmt.Errorf("cannot install %s: %w", filepath.Base(m.dest), err) + } + } + return nil } func safePath(destDir, name string) (string, error) { @@ -106,6 +384,11 @@ func extractTarGz(archivePath, destDir string) error { return err } switch hdr.Typeflag { + case tar.TypeSymlink, tar.TypeLink: + // Skipped deliberately, not by omission. No release archive ships + // links, and one appearing here would mean a malformed or hostile + // archive; see extractZip for the concrete hazard. + continue case tar.TypeDir: if err := os.MkdirAll(target, 0o755); err != nil { return err @@ -123,6 +406,12 @@ func extractTarGz(archivePath, destDir string) error { return err } _ = out.Close() + // OpenFile's mode is masked by the process umask, and + // discoverMembers keys extension discovery off the extracted exec + // bits; Chmod is not masked, so it restores the archive's mode. + if err := os.Chmod(target, hdr.FileInfo().Mode().Perm()); err != nil { + return err + } } } return nil @@ -146,6 +435,14 @@ func extractZip(archivePath, destDir string) error { } continue } + // A zip symlink entry stores its target as the file body, so writing it + // out as a regular file produces an executable whose contents are a + // path string, which discoverMembers would then install as an + // extension. No release archive ships links, so skipping is free; the + // check exists so a malformed archive cannot install junk. + if f.Mode()&os.ModeSymlink != 0 { + continue + } if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { return err } @@ -165,10 +462,25 @@ func extractZip(archivePath, destDir string) error { } _ = out.Close() _ = rc.Close() + // Same umask concern as extractTarGz: restore the archive's mode so + // extension discovery sees the exec bits the release shipped. + if err := os.Chmod(target, f.Mode().Perm()); err != nil { + return err + } } return nil } +// copyFile copies src to a NEW file at dst with the given mode, refusing to +// overwrite anything (O_EXCL, which also never follows a symlink). It flushes +// to disk and reports a close failure as an error: a full disk surfaces only +// at flush or close, and a copy that silently half-succeeded would be +// committed over a working file. +// +// The mode is applied with an explicit Chmod after the bytes are safely on +// disk, because the permission argument to OpenFile is masked by the process +// umask and drops the special bits; Chmod is subject to neither, so the file +// ends up with exactly the requested mode, setuid/setgid/sticky included. func copyFile(src, dst string, mode os.FileMode) error { in, err := os.Open(src) if err != nil { @@ -176,12 +488,21 @@ func copyFile(src, dst string, mode os.FileMode) error { } defer func() { _ = in.Close() }() - out, err := os.OpenFile(dst, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, mode) + out, err := os.OpenFile(dst, os.O_WRONLY|os.O_CREATE|os.O_EXCL, mode.Perm()) if err != nil { return err } - defer func() { _ = out.Close() }() - _, err = io.Copy(out, in) - return err + if _, err := io.Copy(out, in); err != nil { + _ = out.Close() + return err + } + if err := out.Sync(); err != nil { + _ = out.Close() + return err + } + if err := out.Close(); err != nil { + return err + } + return os.Chmod(dst, mode) } diff --git a/internal/update/extract_test.go b/internal/update/extract_test.go new file mode 100644 index 00000000..bc51a3b2 --- /dev/null +++ b/internal/update/extract_test.go @@ -0,0 +1,724 @@ +package update + +import ( + "archive/tar" + "archive/zip" + "compress/gzip" + "os" + "path/filepath" + goruntime "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// archiveFormats are the two release archive formats the updater accepts. +// Every set-replacement behavior is asserted against both, because goreleaser +// ships zip on Windows and tar.gz everywhere else and the two extractors are +// separate code paths. +var archiveFormats = []string{"tar.gz", "zip"} + +type archiveEntry struct { + name string + body string + mode os.FileMode + // link, when set, makes this a symlink entry pointing at the given target + // instead of a regular file. Release archives never contain one; it exists + // to build the malformed archives the extractors must reject. + link string +} + +// lstkBinaryName is the archive-root name of the lstk binary on this platform. +func lstkBinaryName() string { + return exeName("lstk", goruntime.GOOS) +} + +// extBinaryName is the archive-root name of the bundled extension providing +// the given command on this platform. +func extBinaryName(name string) string { + return exeName("lstk-"+name, goruntime.GOOS) +} + +// buildArchive writes a release-shaped archive containing exactly the given +// entries at its root and returns its path. +func buildArchive(t *testing.T, format string, entries []archiveEntry) string { + t.Helper() + path := filepath.Join(t.TempDir(), "archive."+format) + f, err := os.Create(path) + require.NoError(t, err) + defer func() { require.NoError(t, f.Close()) }() + + if format == "zip" { + zw := zip.NewWriter(f) + for _, e := range entries { + hdr := &zip.FileHeader{Name: e.name, Method: zip.Deflate} + body := e.body + if e.link != "" { + // A zip symlink is a mode-flagged entry whose body is the target. + hdr.SetMode(e.mode | os.ModeSymlink) + body = e.link + } else { + hdr.SetMode(e.mode) + } + w, err := zw.CreateHeader(hdr) + require.NoError(t, err) + _, err = w.Write([]byte(body)) + require.NoError(t, err) + } + require.NoError(t, zw.Close()) + return path + } + + gw := gzip.NewWriter(f) + tw := tar.NewWriter(gw) + for _, e := range entries { + if e.link != "" { + require.NoError(t, tw.WriteHeader(&tar.Header{ + Name: e.name, + Mode: int64(e.mode), + Linkname: e.link, + Typeflag: tar.TypeSymlink, + })) + continue + } + require.NoError(t, tw.WriteHeader(&tar.Header{ + Name: e.name, + Mode: int64(e.mode), + Size: int64(len(e.body)), + Typeflag: tar.TypeReg, + })) + _, err := tw.Write([]byte(e.body)) + require.NoError(t, err) + } + require.NoError(t, tw.Close()) + require.NoError(t, gw.Close()) + return path +} + +// newInstallDir returns a fresh install directory containing the given files +// plus the path the running lstk binary would occupy. +func newInstallDir(t *testing.T, files map[string]string) (dir, exePath string) { + t.Helper() + dir = t.TempDir() + for name, body := range files { + mode := os.FileMode(0o644) + if name != descriptionsFileName { + mode = 0o755 + } + require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte(body), mode)) + } + return dir, filepath.Join(dir, lstkBinaryName()) +} + +func requireFileContent(t *testing.T, path, want string) { + t.Helper() + got, err := os.ReadFile(path) + require.NoError(t, err, "expected %s to exist", path) + assert.Equal(t, want, string(got), "content of %s", path) +} + +func requireExecutable(t *testing.T, path string) { + t.Helper() + info, err := os.Stat(path) + require.NoError(t, err) + if goruntime.GOOS == "windows" { + // Windows has no execute bit; executability is decided by the .exe + // suffix, which the archive-root name already carries. + return + } + assert.NotZero(t, info.Mode().Perm()&0o111, "%s should be executable, got %v", path, info.Mode().Perm()) +} + +// requireNoStagingLeftovers asserts the install directory holds no staging +// files, which is what makes a later re-run a clean repair rather than a +// resume of unknown state. +func requireNoStagingLeftovers(t *testing.T, dir string) { + t.Helper() + matches, err := filepath.Glob(filepath.Join(dir, "*"+stagingSuffix)) + require.NoError(t, err) + var files []string + for _, m := range matches { + info, err := os.Stat(m) + require.NoError(t, err) + if info.Mode().IsRegular() { + files = append(files, m) + } + } + assert.Empty(t, files, "no staging files should be left behind") +} + +// TestExtractAndReplaceReplacesWholeSet covers the core promise of the +// set-wise updater: lstk, every bundled extension, and the descriptions file +// are replaced together by one update. +func TestExtractAndReplaceReplacesWholeSet(t *testing.T) { + t.Parallel() + for _, format := range archiveFormats { + t.Run(format, func(t *testing.T) { + t.Parallel() + dir, exePath := newInstallDir(t, map[string]string{ + lstkBinaryName(): "old lstk", + extBinaryName("alpha"): "old alpha", + extBinaryName("beta"): "old beta", + descriptionsFileName: "alpha = \"old alpha\"\n", + }) + + archive := buildArchive(t, format, []archiveEntry{ + {name: lstkBinaryName(), body: "new lstk", mode: 0o755}, + {name: extBinaryName("alpha"), body: "new alpha", mode: 0o755}, + {name: extBinaryName("beta"), body: "new beta", mode: 0o755}, + {name: descriptionsFileName, body: "alpha = \"new alpha\"\nbeta = \"new beta\"\n", mode: 0o644}, + }) + + require.NoError(t, extractAndReplace(archive, exePath, format)) + + requireFileContent(t, exePath, "new lstk") + requireFileContent(t, filepath.Join(dir, extBinaryName("alpha")), "new alpha") + requireFileContent(t, filepath.Join(dir, extBinaryName("beta")), "new beta") + requireFileContent(t, filepath.Join(dir, descriptionsFileName), "alpha = \"new alpha\"\nbeta = \"new beta\"\n") + requireExecutable(t, exePath) + requireExecutable(t, filepath.Join(dir, extBinaryName("alpha"))) + requireExecutable(t, filepath.Join(dir, extBinaryName("beta"))) + requireNoStagingLeftovers(t, dir) + }) + } +} + +// TestExtractAndReplaceInstallsNewExtension covers a release that adds an +// extension the install did not have. +func TestExtractAndReplaceInstallsNewExtension(t *testing.T) { + t.Parallel() + for _, format := range archiveFormats { + t.Run(format, func(t *testing.T) { + t.Parallel() + dir, exePath := newInstallDir(t, map[string]string{ + lstkBinaryName(): "old lstk", + }) + + archive := buildArchive(t, format, []archiveEntry{ + {name: lstkBinaryName(), body: "new lstk", mode: 0o755}, + {name: extBinaryName("deploy"), body: "new deploy", mode: 0o755}, + {name: descriptionsFileName, body: "deploy = \"Deploy\"\n", mode: 0o644}, + }) + + require.NoError(t, extractAndReplace(archive, exePath, format)) + + requireFileContent(t, exePath, "new lstk") + requireFileContent(t, filepath.Join(dir, extBinaryName("deploy")), "new deploy") + requireExecutable(t, filepath.Join(dir, extBinaryName("deploy"))) + requireFileContent(t, filepath.Join(dir, descriptionsFileName), "deploy = \"Deploy\"\n") + requireNoStagingLeftovers(t, dir) + }) + } +} + +// TestExtractAndReplaceLstkOnlyArchive pins the pre-bundling and rollback +// shape: an archive carrying only lstk is a valid set of size one and must +// behave exactly as the updater did before bundling existed. +func TestExtractAndReplaceLstkOnlyArchive(t *testing.T) { + t.Parallel() + for _, format := range archiveFormats { + t.Run(format, func(t *testing.T) { + t.Parallel() + dir, exePath := newInstallDir(t, map[string]string{ + lstkBinaryName(): "old lstk", + extBinaryName("alpha"): "installed alpha", + }) + + archive := buildArchive(t, format, []archiveEntry{ + {name: lstkBinaryName(), body: "new lstk", mode: 0o755}, + }) + + require.NoError(t, extractAndReplace(archive, exePath, format)) + + requireFileContent(t, exePath, "new lstk") + // Rollback leaves previously installed extensions in place and runnable. + requireFileContent(t, filepath.Join(dir, extBinaryName("alpha")), "installed alpha") + requireExecutable(t, filepath.Join(dir, extBinaryName("alpha"))) + requireNoStagingLeftovers(t, dir) + }) + } +} + +// TestExtractAndReplaceLeavesUnmatchedExtensionAlone pins the additive-only +// rule: the updater cannot tell a dropped bundled extension from one the user +// placed there, so it never deletes an lstk-* file absent from the archive. +func TestExtractAndReplaceLeavesUnmatchedExtensionAlone(t *testing.T) { + t.Parallel() + dir, exePath := newInstallDir(t, map[string]string{ + lstkBinaryName(): "old lstk", + extBinaryName("mine"): "user extension", + }) + + archive := buildArchive(t, "tar.gz", []archiveEntry{ + {name: lstkBinaryName(), body: "new lstk", mode: 0o755}, + {name: extBinaryName("deploy"), body: "new deploy", mode: 0o755}, + {name: descriptionsFileName, body: "deploy = \"Deploy\"\n", mode: 0o644}, + }) + + require.NoError(t, extractAndReplace(archive, exePath, "tar.gz")) + + requireFileContent(t, filepath.Join(dir, extBinaryName("mine")), "user extension") + requireFileContent(t, filepath.Join(dir, extBinaryName("deploy")), "new deploy") +} + +// TestExtractAndReplaceStagingFailureLeavesInstallUntouched proves a failure +// before commit cannot produce a partial set: nothing under a final name +// changes, and no staging files survive. +func TestExtractAndReplaceStagingFailureLeavesInstallUntouched(t *testing.T) { + t.Parallel() + dir, exePath := newInstallDir(t, map[string]string{ + lstkBinaryName(): "old lstk", + extBinaryName("alpha"): "old alpha", + extBinaryName("beta"): "old beta", + descriptionsFileName: "alpha = \"old\"\n", + }) + + // A non-empty directory squatting on beta's staging path makes its copy + // fail after alpha has already been staged: a failure partway through. + blocked := filepath.Join(dir, extBinaryName("beta")+stagingSuffix) + require.NoError(t, os.MkdirAll(blocked, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(blocked, "occupied"), []byte("x"), 0o644)) + + archive := buildArchive(t, "tar.gz", []archiveEntry{ + {name: lstkBinaryName(), body: "new lstk", mode: 0o755}, + {name: extBinaryName("alpha"), body: "new alpha", mode: 0o755}, + {name: extBinaryName("beta"), body: "new beta", mode: 0o755}, + {name: descriptionsFileName, body: "alpha = \"new\"\n", mode: 0o644}, + }) + + err := extractAndReplace(archive, exePath, "tar.gz") + require.Error(t, err) + assert.Contains(t, err.Error(), extBinaryName("beta"), "error should name the member that failed") + + requireFileContent(t, exePath, "old lstk") + requireFileContent(t, filepath.Join(dir, extBinaryName("alpha")), "old alpha") + requireFileContent(t, filepath.Join(dir, extBinaryName("beta")), "old beta") + requireFileContent(t, filepath.Join(dir, descriptionsFileName), "alpha = \"old\"\n") + requireNoStagingLeftovers(t, dir) +} + +// TestExtractAndReplaceCleansLeftoverStagingFiles proves that re-running +// `lstk update` repairs an update that crashed partway through: leftover +// staging files from the crashed run are removed before the new one stages. +func TestExtractAndReplaceCleansLeftoverStagingFiles(t *testing.T) { + t.Parallel() + dir, exePath := newInstallDir(t, map[string]string{ + lstkBinaryName(): "old lstk", + }) + + // Leftovers a crashed update would have left behind, including one for a + // member the new archive does not carry. + for _, name := range []string{lstkBinaryName(), extBinaryName("alpha"), extBinaryName("gone")} { + require.NoError(t, os.WriteFile(filepath.Join(dir, name+stagingSuffix), []byte("crashed"), 0o755)) + } + + archive := buildArchive(t, "tar.gz", []archiveEntry{ + {name: lstkBinaryName(), body: "new lstk", mode: 0o755}, + {name: extBinaryName("alpha"), body: "new alpha", mode: 0o755}, + }) + + require.NoError(t, extractAndReplace(archive, exePath, "tar.gz")) + + requireFileContent(t, exePath, "new lstk") + requireFileContent(t, filepath.Join(dir, extBinaryName("alpha")), "new alpha") + requireNoStagingLeftovers(t, dir) + // The stale leftover must not have been committed under a real name. + _, err := os.Stat(filepath.Join(dir, extBinaryName("gone"))) + assert.True(t, os.IsNotExist(err), "a leftover staging file must never become a real member") +} + +// TestExtractAndReplaceCommitFailureKeepsPreviousLstk is the case that +// justifies committing lstk last: when a member fails to commit, the user is +// left on their previous, complete version rather than a new lstk with an +// incomplete set. +func TestExtractAndReplaceCommitFailureKeepsPreviousLstk(t *testing.T) { + t.Parallel() + dir, exePath := newInstallDir(t, map[string]string{ + lstkBinaryName(): "old lstk", + }) + + // A non-empty directory under alpha's final name makes its commit rename + // fail, after staging has fully succeeded. + blocked := filepath.Join(dir, extBinaryName("alpha")) + require.NoError(t, os.MkdirAll(blocked, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(blocked, "occupied"), []byte("x"), 0o644)) + + archive := buildArchive(t, "tar.gz", []archiveEntry{ + {name: lstkBinaryName(), body: "new lstk", mode: 0o755}, + {name: extBinaryName("alpha"), body: "new alpha", mode: 0o755}, + {name: descriptionsFileName, body: "alpha = \"new\"\n", mode: 0o644}, + }) + + err := extractAndReplace(archive, exePath, "tar.gz") + require.Error(t, err) + assert.Contains(t, err.Error(), extBinaryName("alpha"), "error should name the member that failed") + + // lstk must still be the previous version. + requireFileContent(t, exePath, "old lstk") +} + +// TestExtractAndReplaceMissingBinary keeps the pre-existing contract that an +// archive without the lstk binary is rejected. +func TestExtractAndReplaceMissingBinary(t *testing.T) { + t.Parallel() + dir, exePath := newInstallDir(t, map[string]string{ + lstkBinaryName(): "old lstk", + }) + + archive := buildArchive(t, "tar.gz", []archiveEntry{ + {name: extBinaryName("alpha"), body: "new alpha", mode: 0o755}, + }) + + err := extractAndReplace(archive, exePath, "tar.gz") + require.Error(t, err) + assert.Contains(t, err.Error(), "binary not found in archive") + requireFileContent(t, exePath, "old lstk") + requireNoStagingLeftovers(t, dir) +} + +// TestExtractAndReplaceIgnoresNonExecutableArchiveEntries proves the discovery +// rule does not mistake a data file shipped next to the binaries for an +// extension. +func TestExtractAndReplaceIgnoresNonExecutableArchiveEntries(t *testing.T) { + t.Parallel() + dir, exePath := newInstallDir(t, map[string]string{ + lstkBinaryName(): "old lstk", + }) + + archive := buildArchive(t, "tar.gz", []archiveEntry{ + {name: lstkBinaryName(), body: "new lstk", mode: 0o755}, + {name: "lstk-notes.txt", body: "not an extension", mode: 0o644}, + {name: "README.md", body: "readme", mode: 0o644}, + {name: "LICENSE", body: "license", mode: 0o644}, + }) + + require.NoError(t, extractAndReplace(archive, exePath, "tar.gz")) + + requireFileContent(t, exePath, "new lstk") + for _, name := range []string{"lstk-notes.txt", "README.md", "LICENSE"} { + _, err := os.Stat(filepath.Join(dir, name)) + assert.True(t, os.IsNotExist(err), "%s must not be installed", name) + } +} + +// TestReplaceSetWindowsVariant exercises the Windows shape of an update: +// zip archive, ".exe" names, and moving the running lstk.exe aside before +// replacing it, from any host. Unit tests run on Linux only in CI, so without +// the goos parameter this path would ship untested. +func TestReplaceSetWindowsVariant(t *testing.T) { + t.Parallel() + dir := t.TempDir() + exePath := filepath.Join(dir, "lstk.exe") + require.NoError(t, os.WriteFile(exePath, []byte("old lstk"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "lstk-alpha.exe"), []byte("old alpha"), 0o755)) + + archive := buildArchive(t, "zip", []archiveEntry{ + {name: "lstk.exe", body: "new lstk", mode: 0o755}, + {name: "lstk-alpha.exe", body: "new alpha", mode: 0o755}, + {name: "lstk-beta.exe", body: "new beta", mode: 0o755}, + // Neither of these is an extension on Windows: executability there is + // decided by the suffix, not by a mode bit the archive cannot carry. + {name: "lstk-notes.txt", body: "not an extension", mode: 0o644}, + {name: "lstk-plain", body: "not an extension either", mode: 0o755}, + {name: descriptionsFileName, body: "alpha = \"Alpha\"\nbeta = \"Beta\"\n", mode: 0o644}, + }) + + require.NoError(t, replaceSet(archive, exePath, "zip", "windows")) + + requireFileContent(t, exePath, "new lstk") + requireFileContent(t, filepath.Join(dir, "lstk-alpha.exe"), "new alpha") + requireFileContent(t, filepath.Join(dir, "lstk-beta.exe"), "new beta") + requireFileContent(t, filepath.Join(dir, descriptionsFileName), "alpha = \"Alpha\"\nbeta = \"Beta\"\n") + // The running binary is renamed aside rather than overwritten. + requireFileContent(t, filepath.Join(dir, "lstk.exe.old"), "old lstk") + for _, name := range []string{"lstk-notes.txt", "lstk-plain"} { + _, err := os.Stat(filepath.Join(dir, name)) + assert.True(t, os.IsNotExist(err), "%s must not be installed as an extension", name) + } + requireNoStagingLeftovers(t, dir) +} + +// TestCommitRestoresRunningBinaryOnWindowsRenameFailure covers the one gap in +// "re-run lstk update to repair" on Windows: the running lstk.exe is moved +// aside before the final rename, so a failure there would leave nothing under +// the real name, and no lstk to re-run. The commit must move it back. +func TestCommitRestoresRunningBinaryOnWindowsRenameFailure(t *testing.T) { + t.Parallel() + dir := t.TempDir() + dest := filepath.Join(dir, "lstk.exe") + require.NoError(t, os.WriteFile(dest, []byte("old lstk"), 0o755)) + + // No staging file exists, so the final rename fails after the aside-move. + m := updateMember{dest: dest, mode: 0o755} + require.Error(t, m.commit("windows")) + + requireFileContent(t, dest, "old lstk") + _, err := os.Stat(dest + ".old") + assert.True(t, os.IsNotExist(err), "the moved-aside binary should have been moved back") +} + +// TestReplaceSetWindowsVariantLstkOnlyArchive pins the rollback shape on the +// Windows path, including the second run's cleanup of the leftover .old file. +func TestReplaceSetWindowsVariantLstkOnlyArchive(t *testing.T) { + t.Parallel() + dir := t.TempDir() + exePath := filepath.Join(dir, "lstk.exe") + require.NoError(t, os.WriteFile(exePath, []byte("old lstk"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "lstk.exe.old"), []byte("older lstk"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "lstk-alpha.exe"), []byte("installed alpha"), 0o755)) + + archive := buildArchive(t, "zip", []archiveEntry{{name: "lstk.exe", body: "new lstk", mode: 0o755}}) + + require.NoError(t, replaceSet(archive, exePath, "zip", "windows")) + + requireFileContent(t, exePath, "new lstk") + requireFileContent(t, filepath.Join(dir, "lstk.exe.old"), "old lstk") + requireFileContent(t, filepath.Join(dir, "lstk-alpha.exe"), "installed alpha") + requireNoStagingLeftovers(t, dir) +} + +// TestExtractAndReplaceInstallsBundle covers the shape a bundling release +// actually ships: the single multi-call binary and the descriptions file, both +// installed as ordinary members of the set. +func TestExtractAndReplaceInstallsBundle(t *testing.T) { + t.Parallel() + for _, format := range archiveFormats { + t.Run(format, func(t *testing.T) { + t.Parallel() + dir, exePath := newInstallDir(t, map[string]string{ + lstkBinaryName(): "old lstk", + bundledBinaryBaseName: "old multi-call binary", + descriptionsFileName: "deploy = \"Deploy\"\n", + }) + + archive := buildArchive(t, format, []archiveEntry{ + {name: lstkBinaryName(), body: "new lstk", mode: 0o755}, + {name: bundledBinaryBaseName, body: "new multi-call binary", mode: 0o755}, + {name: descriptionsFileName, body: "deploy = \"Deploy\"\ndoctor = \"Doctor\"\n", mode: 0o644}, + }) + + require.NoError(t, extractAndReplace(archive, exePath, format)) + + requireFileContent(t, exePath, "new lstk") + requireFileContent(t, filepath.Join(dir, bundledBinaryBaseName), "new multi-call binary") + requireExecutable(t, filepath.Join(dir, bundledBinaryBaseName)) + requireFileContent(t, filepath.Join(dir, descriptionsFileName), "deploy = \"Deploy\"\ndoctor = \"Doctor\"\n") + requireNoStagingLeftovers(t, dir) + }) + } +} + +// TestExtractAndReplaceInstallsBundleWithoutDescriptions covers a bundle whose +// descriptions file is absent: the binary still installs and the update +// succeeds, since the descriptions file only affects help rendering. +func TestExtractAndReplaceInstallsBundleWithoutDescriptions(t *testing.T) { + t.Parallel() + dir, exePath := newInstallDir(t, map[string]string{ + lstkBinaryName(): "old lstk", + }) + + archive := buildArchive(t, "tar.gz", []archiveEntry{ + {name: lstkBinaryName(), body: "new lstk", mode: 0o755}, + {name: bundledBinaryBaseName, body: "multi-call binary", mode: 0o755}, + }) + + require.NoError(t, extractAndReplace(archive, exePath, "tar.gz")) + + requireFileContent(t, exePath, "new lstk") + requireFileContent(t, filepath.Join(dir, bundledBinaryBaseName), "multi-call binary") + requireNoStagingLeftovers(t, dir) +} + +// TestExtractorsSkipSymlinkEntries keeps a malformed or hostile archive from +// installing junk. No release archive ships links, but a zip symlink entry +// stores its target as the file body, so extracting one as a regular file +// yields an executable whose contents are a path string, which discoverMembers +// would then install as a bundled extension. +func TestExtractorsSkipSymlinkEntries(t *testing.T) { + t.Parallel() + for _, format := range archiveFormats { + t.Run(format, func(t *testing.T) { + t.Parallel() + archive := buildArchive(t, format, []archiveEntry{ + {name: lstkBinaryName(), body: "new lstk", mode: 0o755}, + {name: bundledBinaryBaseName, body: "multi-call binary", mode: 0o755}, + {name: "lstk-deploy", mode: 0o755, link: bundledBinaryBaseName}, + }) + + dest := t.TempDir() + if format == "zip" { + require.NoError(t, extractZip(archive, dest)) + } else { + require.NoError(t, extractTarGz(archive, dest)) + } + + _, err := os.Lstat(filepath.Join(dest, "lstk-deploy")) + assert.True(t, os.IsNotExist(err), "an archive symlink entry must not be materialized") + requireFileContent(t, filepath.Join(dest, bundledBinaryBaseName), "multi-call binary") + requireFileContent(t, filepath.Join(dest, lstkBinaryName()), "new lstk") + }) + } +} + +// TestStagingRefusesSymlinkSquatter guards the promise the cleanup step makes: +// a non-regular file under a staging name belongs to the user and is not +// deleted. Staging must then refuse to write through it. Without the refusal, +// the copy follows the symlink and destroys whatever it points at, and the +// commit installs the symlink itself as the member. +func TestStagingRefusesSymlinkSquatter(t *testing.T) { + t.Parallel() + skipIfNoSymlinksSquatter(t) + dir, exePath := newInstallDir(t, map[string]string{ + lstkBinaryName(): "old lstk", + }) + + target := filepath.Join(t.TempDir(), "precious") + require.NoError(t, os.WriteFile(target, []byte("precious data"), 0o644)) + require.NoError(t, os.Symlink(target, filepath.Join(dir, extBinaryName("alpha")+stagingSuffix))) + + archive := buildArchive(t, "tar.gz", []archiveEntry{ + {name: lstkBinaryName(), body: "new lstk", mode: 0o755}, + {name: extBinaryName("alpha"), body: "new alpha", mode: 0o755}, + }) + + err := extractAndReplace(archive, exePath, "tar.gz") + require.Error(t, err, "staging must refuse to write through a squatting symlink") + assert.Contains(t, err.Error(), extBinaryName("alpha"), "error should name the member") + + requireFileContent(t, target, "precious data") + requireFileContent(t, exePath, "old lstk") + info, lerr := os.Lstat(filepath.Join(dir, extBinaryName("alpha"))) + if lerr == nil { + t.Fatalf("no file should exist under the member's final name, found mode %v", info.Mode()) + } +} + +func skipIfNoSymlinksSquatter(t *testing.T) { + t.Helper() + if goruntime.GOOS == "windows" { + t.Skip("os.Symlink needs Developer Mode or elevation on Windows") + } +} + +// TestStagingRefusesDirectorySquatter: a directory under a staging name cannot +// be staged over and must produce an error that tells the user what to move, +// not a raw open failure that blames the filesystem. +func TestStagingRefusesDirectorySquatter(t *testing.T) { + t.Parallel() + dir, exePath := newInstallDir(t, map[string]string{ + lstkBinaryName(): "old lstk", + }) + require.NoError(t, os.MkdirAll(filepath.Join(dir, extBinaryName("alpha")+stagingSuffix), 0o755)) + + archive := buildArchive(t, "tar.gz", []archiveEntry{ + {name: lstkBinaryName(), body: "new lstk", mode: 0o755}, + {name: extBinaryName("alpha"), body: "new alpha", mode: 0o755}, + }) + + err := extractAndReplace(archive, exePath, "tar.gz") + require.Error(t, err) + assert.Contains(t, err.Error(), "move it out of the way", "the error should tell the user how to recover") + requireFileContent(t, exePath, "old lstk") +} + +// TestStagingRefusesConcurrentStagingFile: a regular file appearing at a +// staging path after cleanup means another update is running. Staging must +// fail rather than truncate the other process's half-written copy, which the +// other process would then commit under the real name. +func TestStagingRefusesConcurrentStagingFile(t *testing.T) { + t.Parallel() + dir := t.TempDir() + dest := filepath.Join(dir, "lstk-alpha") + src := filepath.Join(t.TempDir(), "src") + require.NoError(t, os.WriteFile(src, []byte("new alpha"), 0o755)) + require.NoError(t, os.WriteFile(dest+stagingSuffix, []byte("another update's bytes"), 0o755)) + + err := stageMembers([]updateMember{{src: src, dest: dest, mode: 0o755}}) + require.Error(t, err, "staging must not overwrite an existing staging file") + requireFileContent(t, dest+stagingSuffix, "another update's bytes") +} + +// TestUpdateWorksInGlobMetacharacterDir: the install directory path is data, +// not a pattern. A '[' in the path must neither fail the update nor disable +// the leftover cleanup. +func TestUpdateWorksInGlobMetacharacterDir(t *testing.T) { + t.Parallel() + for _, dirName := range []string{"we[ird", "we[ir]d", "sta*rs", "quest?ion"} { + t.Run(dirName, func(t *testing.T) { + t.Parallel() + dir := filepath.Join(t.TempDir(), dirName) + require.NoError(t, os.MkdirAll(dir, 0o755)) + exePath := filepath.Join(dir, lstkBinaryName()) + require.NoError(t, os.WriteFile(exePath, []byte("old lstk"), 0o755)) + leftover := filepath.Join(dir, extBinaryName("gone")+stagingSuffix) + require.NoError(t, os.WriteFile(leftover, []byte("crashed"), 0o755)) + + archive := buildArchive(t, "tar.gz", []archiveEntry{ + {name: lstkBinaryName(), body: "new lstk", mode: 0o755}, + }) + + require.NoError(t, extractAndReplace(archive, exePath, "tar.gz")) + requireFileContent(t, exePath, "new lstk") + _, err := os.Stat(leftover) + assert.True(t, os.IsNotExist(err), "leftover staging files must be cleaned in %q", dirName) + }) + } +} + +// TestUpdatePreservesSpecialModeBits: an lstk installed with setgid (or +// setuid/sticky) must keep those bits across an update, as it did before the +// set-wise updater. +func TestUpdatePreservesSpecialModeBits(t *testing.T) { + t.Parallel() + if goruntime.GOOS == "windows" { + t.Skip("no Unix mode bits on Windows") + } + dir, exePath := newInstallDir(t, map[string]string{ + lstkBinaryName(): "old lstk", + }) + _ = dir + require.NoError(t, os.Chmod(exePath, 0o755|os.ModeSetgid)) + info, err := os.Stat(exePath) + require.NoError(t, err) + if info.Mode()&os.ModeSetgid == 0 { + t.Skip("filesystem does not support setgid on files") + } + + archive := buildArchive(t, "tar.gz", []archiveEntry{ + {name: lstkBinaryName(), body: "new lstk", mode: 0o755}, + }) + require.NoError(t, extractAndReplace(archive, exePath, "tar.gz")) + + info, err = os.Stat(exePath) + require.NoError(t, err) + assert.NotZero(t, info.Mode()&os.ModeSetgid, "setgid bit must survive the update, got mode %v", info.Mode()) +} + +// TestReplaceSetWindowsMovesExistingMembersAside: on Windows every existing +// member gets the move-aside treatment, not only the running lstk.exe. A user +// can be running a bundled extension while the update commits, and a running +// executable can be renamed but not replaced there. +func TestReplaceSetWindowsMovesExistingMembersAside(t *testing.T) { + t.Parallel() + dir := t.TempDir() + exePath := filepath.Join(dir, "lstk.exe") + require.NoError(t, os.WriteFile(exePath, []byte("old lstk"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "lstk-alpha.exe"), []byte("old alpha"), 0o755)) + + archive := buildArchive(t, "zip", []archiveEntry{ + {name: "lstk.exe", body: "new lstk", mode: 0o755}, + {name: "lstk-alpha.exe", body: "new alpha", mode: 0o755}, + {name: "lstk-beta.exe", body: "new beta", mode: 0o755}, + }) + + require.NoError(t, replaceSet(archive, exePath, "zip", "windows")) + + requireFileContent(t, filepath.Join(dir, "lstk-alpha.exe"), "new alpha") + requireFileContent(t, filepath.Join(dir, "lstk-alpha.exe.old"), "old alpha") + requireFileContent(t, filepath.Join(dir, "lstk-beta.exe"), "new beta") + // A member that did not exist before has nothing to move aside. + _, err := os.Stat(filepath.Join(dir, "lstk-beta.exe.old")) + assert.True(t, os.IsNotExist(err)) + requireFileContent(t, filepath.Join(dir, "lstk.exe.old"), "old lstk") +} diff --git a/internal/update/extract_unix_test.go b/internal/update/extract_unix_test.go new file mode 100644 index 00000000..8fc39d4c --- /dev/null +++ b/internal/update/extract_unix_test.go @@ -0,0 +1,49 @@ +//go:build !windows + +package update + +import ( + "os" + "path/filepath" + "syscall" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestExtractorsPreserveArchiveModesRegardlessOfUmask pins that extraction +// restores the archive's file modes explicitly: the mode passed to OpenFile is +// masked by the process umask, and discoverMembers keys extension discovery +// off the extracted exec bits, so without the explicit Chmod a restrictive +// umask would silently shrink the installed set. +// +// Not parallel: umask is process-wide, and Go releases paused parallel tests +// only after the sequential pass finishes, so changing it here cannot race +// another test's file creation. +func TestExtractorsPreserveArchiveModesRegardlessOfUmask(t *testing.T) { + old := syscall.Umask(0o077) + defer syscall.Umask(old) + + for _, format := range archiveFormats { + t.Run(format, func(t *testing.T) { + archive := buildArchive(t, format, []archiveEntry{ + {name: "lstk", body: "new lstk", mode: 0o755}, + {name: "lstk-alpha", body: "new alpha", mode: 0o755}, + }) + + dest := t.TempDir() + if format == "zip" { + require.NoError(t, extractZip(archive, dest)) + } else { + require.NoError(t, extractTarGz(archive, dest)) + } + + for _, name := range []string{"lstk", "lstk-alpha"} { + info, err := os.Stat(filepath.Join(dest, name)) + require.NoError(t, err) + require.Equal(t, os.FileMode(0o755), info.Mode().Perm(), + "%s should keep its archive mode despite the umask", name) + } + }) + } +} diff --git a/internal/update/github_test.go b/internal/update/github_test.go index 8af7321d..b9eb8ea1 100644 --- a/internal/update/github_test.go +++ b/internal/update/github_test.go @@ -1,10 +1,6 @@ package update import ( - "archive/tar" - "archive/zip" - "bytes" - "compress/gzip" "context" "crypto/sha256" "encoding/hex" @@ -20,44 +16,24 @@ import ( // makeReleaseArchive builds an archive in the format updateBinary expects for // the current GOOS (zip on windows, tar.gz elsewhere) containing a single -// binary entry at the archive root. +// binary entry at the archive root. It delegates to buildArchive +// (extract_test.go) so the package has exactly one archive builder and the +// checksum tests and the set-replacement tests cannot drift onto different +// archive shapes. func makeReleaseArchive(t *testing.T, binaryContent string) []byte { t.Helper() - var buf bytes.Buffer + format := "tar.gz" if goruntime.GOOS == "windows" { - zw := zip.NewWriter(&buf) - w, err := zw.Create("lstk.exe") - if err != nil { - t.Fatal(err) - } - if _, err := w.Write([]byte(binaryContent)); err != nil { - t.Fatal(err) - } - if err := zw.Close(); err != nil { - t.Fatal(err) - } - return buf.Bytes() - } - gw := gzip.NewWriter(&buf) - tw := tar.NewWriter(gw) - if err := tw.WriteHeader(&tar.Header{ - Name: "lstk", - Mode: 0o755, - Size: int64(len(binaryContent)), - Typeflag: tar.TypeReg, - }); err != nil { - t.Fatal(err) + format = "zip" } - if _, err := tw.Write([]byte(binaryContent)); err != nil { - t.Fatal(err) - } - if err := tw.Close(); err != nil { - t.Fatal(err) - } - if err := gw.Close(); err != nil { + path := buildArchive(t, format, []archiveEntry{ + {name: exeName("lstk", goruntime.GOOS), body: binaryContent, mode: 0o755}, + }) + data, err := os.ReadFile(path) + if err != nil { t.Fatal(err) } - return buf.Bytes() + return data } // fakeExecutable writes a stand-in for the running binary into its own temp diff --git a/internal/update/notify.go b/internal/update/notify.go index 244420df..f16f6d15 100644 --- a/internal/update/notify.go +++ b/internal/update/notify.go @@ -47,12 +47,27 @@ func checkQuietlyWithVersion(ctx context.Context, githubToken string, currentVer } func NotifyUpdate(ctx context.Context, sink output.Sink, opts NotifyOptions) (exitAfter bool) { - return notifyUpdateWithVersion(ctx, sink, opts, version.Version(), fetchLatestVersion) + return notifyUpdateWithVersion(ctx, sink, opts, version.Version(), fetchLatestVersion, missingBundledMembers) } -func notifyUpdateWithVersion(ctx context.Context, sink output.Sink, opts NotifyOptions, currentVersion string, fetch versionFetcher) (exitAfter bool) { +func notifyUpdateWithVersion(ctx context.Context, sink output.Sink, opts NotifyOptions, currentVersion string, fetch versionFetcher, missingMembers func() []string) (exitAfter bool) { current, latest, available := checkQuietlyWithVersion(ctx, opts.GitHubToken, currentVersion, fetch) if !available { + // Being current is not the whole story: the install can be missing its + // bundled extensions (see missingBundledMembers for how the transition + // leaves it that way). The user this happens to would otherwise never + // be nudged toward the repair, since only `lstk update` itself detects + // it. A note is enough; the interactive update prompt stays reserved + // for version upgrades, whose wording and skip logic assume one. + // The probe runs only when the version check succeeded and matched + // (latest is empty on a dev build or a failed fetch), mirroring the + // gating in checkWithVersion. + if latest != "" && len(missingMembers()) > 0 { + sink.Emit(output.MessageEvent{ + Severity: output.SeverityNote, + Text: "Bundled extensions are missing from this install. Run lstk update to restore them.", + }) + } return false } diff --git a/internal/update/notify_test.go b/internal/update/notify_test.go index 499b0916..7d8a469e 100644 --- a/internal/update/notify_test.go +++ b/internal/update/notify_test.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "strings" "testing" "github.com/localstack/lstk/internal/output" @@ -87,7 +88,7 @@ func TestNotifyUpdateNoUpdateAvailable(t *testing.T) { var events []output.Event sink := output.SinkFunc(func(event output.Event) { events = append(events, event) }) - exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{UpdatePrompt: true}, "v1.0.0", testFetcher(server.URL)) + exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{UpdatePrompt: true}, "v1.0.0", testFetcher(server.URL), noMissingMembers) assert.False(t, exit) assert.Empty(t, events) } @@ -99,7 +100,7 @@ func TestNotifyUpdatePromptDisabled(t *testing.T) { var events []output.Event sink := output.SinkFunc(func(event output.Event) { events = append(events, event) }) - exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{}, "1.0.0", testFetcher(server.URL)) + exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{}, "1.0.0", testFetcher(server.URL), noMissingMembers) assert.False(t, exit) assert.Len(t, events, 1) msg, ok := events[0].(output.MessageEvent) @@ -127,7 +128,7 @@ func TestNotifyUpdatePromptSkip(t *testing.T) { skippedVersion = v return nil }, - }, "1.0.0", testFetcher(server.URL)) + }, "1.0.0", testFetcher(server.URL), noMissingMembers) assert.False(t, exit) assert.Equal(t, "v2.0.0", skippedVersion) } @@ -142,7 +143,7 @@ func TestNotifyUpdateSkippedVersionSuppressesPrompt(t *testing.T) { exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{ UpdatePrompt: true, SkippedVersion: "v2.0.0", - }, "1.0.0", testFetcher(server.URL)) + }, "1.0.0", testFetcher(server.URL), noMissingMembers) assert.False(t, exit) assert.Empty(t, events) } @@ -159,7 +160,7 @@ func TestNotifyUpdatePromptRemind(t *testing.T) { } }) - exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{UpdatePrompt: true}, "1.0.0", testFetcher(server.URL)) + exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{UpdatePrompt: true}, "1.0.0", testFetcher(server.URL), noMissingMembers) assert.False(t, exit) } @@ -180,7 +181,49 @@ func TestNotifyUpdatePromptCancelled(t *testing.T) { } }) - exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{UpdatePrompt: true}, "1.0.0", testFetcher(server.URL)) + exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{UpdatePrompt: true}, "1.0.0", testFetcher(server.URL), noMissingMembers) assert.False(t, exit) } +// noMissingMembers is the completeness probe for tests that exercise the +// version-upgrade paths: the installed set is complete, so only the version +// comparison decides anything. +func noMissingMembers() []string { return nil } + +// TestNotifyUpdateNudgesRepairWhenCurrent covers the transition user: current +// version, bundled set incomplete. The passive notice must point at +// `lstk update` instead of staying silent, or that user is never nudged toward +// the repair at all. +func TestNotifyUpdateNudgesRepairWhenCurrent(t *testing.T) { + server := newTestGitHubServer(t, "v1.0.0") + defer server.Close() + + var events []output.Event + sink := output.SinkFunc(func(event output.Event) { events = append(events, event) }) + exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{}, "1.0.0", testFetcher(server.URL), + func() []string { return []string{"bundled-extensions"} }) + + assert.False(t, exit) + found := false + for _, e := range events { + if msg, ok := e.(output.MessageEvent); ok && strings.Contains(msg.Text, "Bundled extensions are missing") { + assert.Contains(t, msg.Text, "lstk update", "the note should say what to run") + found = true + } + } + assert.True(t, found, "expected a repair note, got %+v", events) +} + +// TestNotifyUpdateStaysQuietWhenCurrentAndComplete pins that the everyday +// up-to-date run stays completely silent. +func TestNotifyUpdateStaysQuietWhenCurrentAndComplete(t *testing.T) { + server := newTestGitHubServer(t, "v1.0.0") + defer server.Close() + + var events []output.Event + sink := output.SinkFunc(func(event output.Event) { events = append(events, event) }) + exit := notifyUpdateWithVersion(context.Background(), sink, NotifyOptions{}, "1.0.0", testFetcher(server.URL), noMissingMembers) + + assert.False(t, exit) + assert.Empty(t, events, "an up-to-date, complete install must produce no output") +} diff --git a/internal/update/update.go b/internal/update/update.go index da399bdb..638f33fb 100644 --- a/internal/update/update.go +++ b/internal/update/update.go @@ -1,9 +1,76 @@ +// Package update implements lstk's self-update: checking GitHub for a newer +// release and applying it through whichever mechanism installed lstk (Homebrew, +// npm, or replacing the binary in place). +// +// # What a binary-channel update installs +// +// An update installs the whole version-matched set a release archive carries +// (the lstk binary, the multi-call "bundled-extensions" binary that provides +// every bundled extension, and the descriptions file), not just lstk. Homebrew +// and npm get this for free by replacing the whole package; the binary channel +// implements it in extract.go as stage-then-commit: every member is copied next +// to its destination under a ".lstk-new" name, and only once all copies succeed +// is each renamed over its final name, lstk last. +// +// # The guarantee +// +// This is not atomicity across files, which POSIX cannot deliver. It is three +// specific promises, and the reason the code is shaped the way it is. Preserve +// them through any refactor: +// +// 1. A file visible under its real name is never truncated or half-written. +// New content is only ever written to a freshly created staging file +// (never through anything already on disk), and a final name only ever +// changes by rename, which is atomic within a directory. +// 2. An interrupted update is repaired by re-running `lstk update`. Nothing +// is committed until every member is staged, staging files left by an +// interrupted run are cleaned up before the next one stages, and lstk +// itself commits last, so an interruption leaves the user with a working +// lstk to re-run the update with. Members committed before an interruption +// stay at the new version; that skew is benign by contract (the extension +// API version only changes on breaking releases) and the re-run resolves +// it. One Windows caveat: the running lstk.exe is renamed aside before the +// new one is renamed in, and a crash between those two renames leaves no +// lstk.exe under the real name. Recovery is renaming lstk.exe.old back by +// hand; the commit error message says so when it can. +// 3. The updater never deletes an "lstk-*" file that the new archive does not +// contain. It cannot distinguish a bundled extension a release dropped +// from one the user put there, and the descriptions file is not an +// ownership manifest (a bundled binary is permitted to have no +// description). The binary channel is therefore additive-only: a dropped +// extension keeps working and shows name-only in help, because the +// replaced descriptions file no longer describes it. Homebrew and npm +// remove such files naturally via whole-package replacement. See design +// Decision 4 of the add-bundled-extension-distribution change for the full +// reasoning. +// +// A deliberate consequence of stage-then-commit: the update needs write +// permission in the install directory. The pre-bundling updater could fall +// back to overwriting a writable binary in place inside a read-only directory, +// but that fallback could leave a half-written file under the real name (the +// exact thing promise 1 forbids) and could never install the extensions +// anyway, so it was removed rather than kept for lstk alone. The staging error +// names the directory when this is what failed. +// +// A release archive carrying only lstk (every pre-bundling release, and any +// rollback to one) is a valid set of size one and installs exactly as it did +// before bundling existed. When an archive does carry extensions they are not +// optional: a member that fails to stage or commit fails the whole update and +// names the member, rather than reporting success with a partial set. +// +// An install can also be current and still incomplete, which no version +// comparison can detect; missingBundledMembers explains when that happens and +// how `lstk update` repairs it. A repair verifies afterwards that the members +// are actually present and fails loudly when the release archive did not +// deliver them, so a stamped-but-not-shipped release cannot loop forever +// behind successful-looking updates. package update import ( "bytes" "context" "fmt" + "path/filepath" "strings" "sync" @@ -11,16 +78,25 @@ import ( "github.com/localstack/lstk/internal/version" ) -// Check reports whether a newer version is available. Returns the latest -// version string and true if an update is available. Always emits exactly one -// UpdateCheckedEvent, whose DevBuild/Available fields tell the sink which of -// the three possible outcomes (dev build skipped / already up to date / an -// update is available) occurred. -func Check(ctx context.Context, sink output.Sink, githubToken string) (string, bool, error) { - current := version.Version() +// Check reports whether `lstk update` has work to do. Returns the latest +// version string, whether an update should be applied, and whether that work +// is a same-version repair of an incomplete bundled set rather than a version +// upgrade (repair implies available). Always emits exactly one +// UpdateCheckedEvent, whose DevBuild/RepairBundled/Available fields tell the +// sink which of the four possible outcomes (dev build skipped / already up to +// date / an update is available / the installed set is incomplete and is being +// repaired) occurred. +func Check(ctx context.Context, sink output.Sink, githubToken string) (latest string, available, repair bool, err error) { + return checkWithVersion(ctx, sink, githubToken, version.Version(), missingBundledMembers) +} + +// checkWithVersion is Check with the running version and the set-completeness +// probe as parameters, mirroring checkQuietlyWithVersion, so both can be driven +// from a test. +func checkWithVersion(ctx context.Context, sink output.Sink, githubToken, current string, missingMembers func() []string) (string, bool, bool, error) { if current == "dev" { sink.Emit(output.UpdateCheckedEvent{CurrentVersion: current, DevBuild: true}) - return "", false, nil + return "", false, false, nil } sink.Emit(output.SpinnerStart("Checking for updates")) @@ -29,35 +105,103 @@ func Check(ctx context.Context, sink output.Sink, githubToken string) (string, b if err != nil { wrapped := fmt.Errorf("failed to check for updates: %w", err) sink.Emit(output.ErrorEvent{Title: wrapped.Error(), Code: output.ErrNetworkError}) - return "", false, output.NewSilentError(wrapped) + return "", false, false, output.NewSilentError(wrapped) } available := normalizeVersion(current) != normalizeVersion(latest) - sink.Emit(output.UpdateCheckedEvent{CurrentVersion: current, LatestVersion: latest, Available: available}) - return latest, available, nil + + // An install can be current and still incomplete; missingBundledMembers + // explains how that happens and why the version comparison alone cannot + // detect it. The probe only runs when the versions match, so the ordinary + // up-to-date path costs exactly what it did before. + repair := false + if !available && len(missingMembers()) > 0 { + available, repair = true, true + } + + sink.Emit(output.UpdateCheckedEvent{ + CurrentVersion: current, + LatestVersion: latest, + Available: available, + RepairBundled: repair, + }) + return latest, available, repair, nil } // Update checks for updates and applies the update if one is available. func Update(ctx context.Context, sink output.Sink, checkOnly bool, githubToken string) error { current := version.Version() - latest, available, err := Check(ctx, sink, githubToken) + latest, available, repair, err := Check(ctx, sink, githubToken) if err != nil { return err } - if !available || checkOnly { + if !available { + if !checkOnly { + cleanupStagingLeftovers() + } + return nil + } + if checkOnly { return nil } + // The checked event states only the finding, because it also renders under + // --check; the reinstall is narrated here, where it actually happens. + if repair { + sink.Emit(output.MessageEvent{ + Severity: output.SeverityNote, + Text: fmt.Sprintf("Reinstalling %s to restore bundled extensions", current), + }) + } + method, err := applyUpdate(ctx, sink, latest, githubToken) if err != nil { sink.Emit(output.ErrorEvent{Title: err.Error(), Code: output.ErrInternal}) return output.NewSilentError(err) } + // A repair re-downloads the running version, so success must mean the + // members are actually there now. Without this re-probe, a release whose + // stamped set names a member its own archive does not carry would make + // every `lstk update` download, "succeed", and detect the member missing + // again, forever, with no signal anywhere. The install itself is fine (the + // archive is authoritative for what an update installs), so this fails the + // repair claim, not the file replacement. + if repair { + if still := missingBundledMembers(); len(still) > 0 { + failure := fmt.Errorf("update did not restore the bundled extensions: the %s release archive does not contain %s", + latest, strings.Join(still, ", ")) + sink.Emit(output.ErrorEvent{ + Title: failure.Error(), + Summary: "This is a packaging problem in the release, not in your installation.", + Code: output.ErrInternal, + Actions: []output.ErrorAction{ + {Label: "Report it at:", Value: "https://github.com/localstack/lstk/issues"}, + }, + }) + return output.NewSilentError(failure) + } + } + sink.Emit(output.UpdateAppliedEvent{CurrentVersion: current, UpdatedVersion: latest, Method: method}) return nil } +// cleanupStagingLeftovers removes staging files in the install directory when +// an up-to-date check decides there is nothing else to do. A repair that was +// interrupted between committing the bundled members and committing lstk +// itself leaves an lstk staging copy behind, and every later run takes the +// up-to-date path (same version, complete set), so without this the leftover +// would sit there until the next release ships. Best effort: failing to tidy a +// leftover is no reason to fail an otherwise satisfied update. +func cleanupStagingLeftovers() { + info := DetectInstallMethod() + if info.Method != InstallBinary || info.ResolvedPath == "" { + return + } + _ = removeStagingFiles(filepath.Dir(info.ResolvedPath)) +} + // applyUpdate detects the current install method and performs the update, // returning its canonical name ("homebrew"/"npm"/"binary") on success. func applyUpdate(ctx context.Context, sink output.Sink, latest, githubToken string) (string, error) { diff --git a/internal/version/version.go b/internal/version/version.go index e2a1f478..30364a22 100644 --- a/internal/version/version.go +++ b/internal/version/version.go @@ -1,7 +1,39 @@ package version +import "strings" + // Set via ldflags at build time. Must be a variable, not a constant, // because the linker can only modify variables at link time. var version = "dev" func Version() string { return version } + +// bundledSet is set via ldflags at build time to the comma-separated +// archive-root names of the bundled-extension files this release ships: the +// multi-call extensions binary and the descriptions file, with ".exe" included +// on Windows builds (e.g. "bundled-extensions,lstk-extensions.toml"). +// +// It is empty on every build that ships no bundle: every release before +// bundled extensions existed, and any release deliberately built without one. +// An empty value keeps `lstk update` on a pure version comparison, which is +// what makes a rollback to an extension-free release behave exactly as it did +// before bundling. +// +// It has to be stamped in rather than derived from disk because a bundling +// release is reachable by pre-bundling updaters, which install only the lstk +// binary and ignore the archive's other members. The install directory they +// leave behind cannot testify to what it should contain, so this is the only +// record the running binary has of the set its own release shipped. +var bundledSet = "" + +// BundledSet returns the archive-root names of the bundled-extension files +// this release ships, or nil when it ships none. +func BundledSet() []string { + var names []string + for _, name := range strings.Split(bundledSet, ",") { + if name = strings.TrimSpace(name); name != "" { + names = append(names, name) + } + } + return names +} diff --git a/openspec/changes/add-bundled-extension-distribution/tasks.md b/openspec/changes/add-bundled-extension-distribution/tasks.md index 272942a0..af1bec39 100644 --- a/openspec/changes/add-bundled-extension-distribution/tasks.md +++ b/openspec/changes/add-bundled-extension-distribution/tasks.md @@ -4,13 +4,13 @@ Today `internal/update/extract.go` extracts the downloaded archive and replaces exactly one file: the `lstk` binary. Once archives also contain extension binaries (`lstk-deploy`, …) and the descriptions file (`lstk-extensions.toml`), the updater has to replace all of them — without ever leaving a half-written file if the update is interrupted. The approach: copy the new files into the install directory under temporary names first (`lstk-deploy.lstk-new`), and only when every copy has succeeded, rename each one over the real name. Renames within a directory are instant and atomic, so nobody can ever run a half-copied binary. -- [ ] 1.1 In `internal/update/extract.go`, build the list of files to replace by looking at the extracted archive root: the lstk binary (`lstk` / `lstk.exe`), every executable file named `lstk-*`, and `lstk-extensions.toml`. If the archive contains only `lstk` (all current releases, and any future rollback), the list has one entry and the updater must behave exactly as it does today. -- [ ] 1.2 Before doing anything else, delete any leftover `*.lstk-new` files in the install directory. These can only exist if a previous update crashed partway through; cleaning them up is what makes "just run `lstk update` again" always repair an interrupted update. -- [ ] 1.3 Copy phase: copy each file from the list into the install directory (the directory of the running executable) under the temporary name `.lstk-new`, and make binaries executable (0755). If any copy fails (disk full, permissions, …), delete the `.lstk-new` files and return an error — the existing installation must be completely untouched. -- [ ] 1.4 Rename phase: once all copies succeeded, rename each `.lstk-new` to ``. Rename the extensions and the toml first and the lstk binary **last**, so if the process dies mid-way the user still has a working lstk and a re-run finishes the job. Keep two existing behaviors as-is: on Windows, the running `lstk.exe` is first moved aside to `lstk.exe.old` (you cannot rename over a running exe there — this applies only to lstk itself, extensions aren't running during an update); and the cross-device copy fallback for installs where rename fails. If a rename fails, stop and return an error naming the file that failed — never report success with only part of the set installed. Renaming lstk last is what makes that safe: any failure before the final rename leaves the user on their previous, complete version. -- [ ] 1.5 Write the resulting guarantee into the package documentation so it survives future refactors: (a) a file visible under its real name is never truncated or half-written, (b) an interrupted update is fixed by re-running `lstk update`, (c) the updater never **deletes** an `lstk-*` file that isn't in the new archive — it can't tell a dropped bundled extension from a file the user put there themselves (design Decision 4 has the full reasoning). -- [ ] 1.6 Re-introduce `internal/update/extract_test.go` with tests that build small tar.gz/zip archives on the fly and cover each behavior above: an archive with lstk + two extensions + toml replaces all of them; an update that introduces a brand-new extension installs it; an archive with only `lstk` reproduces today's behavior; a failed copy leaves the installation untouched; leftover `.lstk-new` files from a fake earlier crash get cleaned up; an `lstk-*` file NOT present in the archive is left alone; a rename failure partway through returns an error and leaves lstk on its previous version; and the Windows zip/`.exe` variant works. -- [ ] 1.7 Repair an incomplete set even when the binary is already current. Anyone crossing the transition on the binary channel gets the new lstk with no extensions, because the updater that ran was their old one which ignores the archive's extra files. They cannot fix that by updating again: `applyUpdate` always jumps straight to the newest release, so they are already on it, and `Check` in `internal/update/update.go` reports "already up to date" until another release ships — leaving them without extensions for up to a week. Make `lstk update` compare the installed set against the set the release is expected to contain, and re-run the install when a member is missing, instead of short-circuiting on the version alone. What the expected set is depends on design Decision 7 — under (b) it is the command list in `lstk-extensions.toml`, under (a) it needs a shipped list, because a directory cannot testify to its own completeness. Cover it in `extract_test.go`/`update_test.go`: a current binary with a missing member installs the member; a current binary with a complete set still reports up to date. +- [x] 1.1 In `internal/update/extract.go`, build the list of files to replace by looking at the extracted archive root: the lstk binary (`lstk` / `lstk.exe`), every executable file named `lstk-*`, and `lstk-extensions.toml`. If the archive contains only `lstk` (all current releases, and any future rollback), the list has one entry and the updater must behave exactly as it does today. +- [x] 1.2 Before doing anything else, delete any leftover `*.lstk-new` files in the install directory. These can only exist if a previous update crashed partway through; cleaning them up is what makes "just run `lstk update` again" always repair an interrupted update. +- [x] 1.3 Copy phase: copy each file from the list into the install directory (the directory of the running executable) under the temporary name `.lstk-new`, and make binaries executable (0755). If any copy fails (disk full, permissions, …), delete the `.lstk-new` files and return an error — the existing installation must be completely untouched. +- [x] 1.4 Rename phase: once all copies succeeded, rename each `.lstk-new` to ``. Rename the extensions and the toml first and the lstk binary **last**, so if the process dies mid-way the user still has a working lstk and a re-run finishes the job. Keep two existing behaviors as-is: on Windows, the running `lstk.exe` is first moved aside to `lstk.exe.old` (you cannot rename over a running exe there — this applies only to lstk itself, extensions aren't running during an update); and the cross-device copy fallback for installs where rename fails. If a rename fails, stop and return an error naming the file that failed — never report success with only part of the set installed. Renaming lstk last is what makes that safe: any failure before the final rename leaves the user on their previous, complete version. +- [x] 1.5 Write the resulting guarantee into the package documentation so it survives future refactors: (a) a file visible under its real name is never truncated or half-written, (b) an interrupted update is fixed by re-running `lstk update`, (c) the updater never **deletes** an `lstk-*` file that isn't in the new archive — it can't tell a dropped bundled extension from a file the user put there themselves (design Decision 4 has the full reasoning). +- [x] 1.6 Re-introduce `internal/update/extract_test.go` with tests that build small tar.gz/zip archives on the fly and cover each behavior above: an archive with lstk + two extensions + toml replaces all of them; an update that introduces a brand-new extension installs it; an archive with only `lstk` reproduces today's behavior; a failed copy leaves the installation untouched; leftover `.lstk-new` files from a fake earlier crash get cleaned up; an `lstk-*` file NOT present in the archive is left alone; a rename failure partway through returns an error and leaves lstk on its previous version; and the Windows zip/`.exe` variant works. +- [x] 1.7 Repair an incomplete set even when the binary is already current. Anyone crossing the transition on the binary channel gets the new lstk with no extensions, because the updater that ran was their old one which ignores the archive's extra files. They cannot fix that by updating again: `applyUpdate` always jumps straight to the newest release, so they are already on it, and `Check` in `internal/update/update.go` reports "already up to date" until another release ships — leaving them without extensions for up to a week. Make `lstk update` compare the installed set against the set the release is expected to contain, and re-run the install when a member is missing, instead of short-circuiting on the version alone. What the expected set is depends on design Decision 7 — under (b) it is the command list in `lstk-extensions.toml`, under (a) it needs a shipped list, because a directory cannot testify to its own completeness. Cover it in `extract_test.go`/`update_test.go`: a current binary with a missing member installs the member; a current binary with a complete set still reports up to date. *Resolved under Decision 7(b):* the toml cannot itself be the completeness signal, since it is one of the members that can be missing, so the expected set is stamped into the binary at build time (`version.bundledSet`, set via an ldflags entry in `.goreleaser.yaml` that the packaging PR adds; an empty stamp preserves pre-bundling behaviour). `lstk update` re-probes after a repair and fails loudly when the archive did not deliver the stamped members, so a stamped-but-not-shipped release cannot loop silently. ## 2. The release-time check that descriptions match binaries diff --git a/test/integration/update_test.go b/test/integration/update_test.go index b85722aa..8e125541 100644 --- a/test/integration/update_test.go +++ b/test/integration/update_test.go @@ -410,11 +410,23 @@ func npmPlatformPackage() string { // buildLstkWithVersion builds the lstk binary from the repo root with the // given version stamped in, writing it to outPath. func buildLstkWithVersion(t *testing.T, ctx context.Context, version, outPath string) { + t.Helper() + buildLstkWithBundledSet(t, ctx, version, "", outPath) +} + +// buildLstkWithBundledSet builds lstk with both release stamps the updater +// reads: the version, and the bundled set this release is expected to ship +// (empty for a release that ships none, which is every release today). +func buildLstkWithBundledSet(t *testing.T, ctx context.Context, version, bundledSet, outPath string) { t.Helper() repoRoot, err := filepath.Abs("../..") require.NoError(t, err) + ldflags := "-X github.com/localstack/lstk/internal/version.version=" + version + if bundledSet != "" { + ldflags += " -X github.com/localstack/lstk/internal/version.bundledSet=" + bundledSet + } buildCmd := exec.CommandContext(ctx, "go", "build", - "-ldflags", "-X github.com/localstack/lstk/internal/version.version="+version, + "-ldflags", ldflags, "-o", outPath, ".", ) @@ -433,32 +445,50 @@ func releaseAssetName(ver string) string { return fmt.Sprintf("lstk_%s_%s_%s.%s", ver, runtime.GOOS, runtime.GOARCH, ext) } +// releaseMember is one file at the root of a release archive. +type releaseMember struct { + name string + body []byte + mode os.FileMode +} + // packageReleaseArchive wraps binary bytes into the release archive format the // updater extracts: a tar.gz (zip on Windows) with a single executable entry. func packageReleaseArchive(t *testing.T, binaryName string, binary []byte) []byte { + t.Helper() + return packageReleaseArchiveWith(t, []releaseMember{{name: binaryName, body: binary, mode: 0o755}}) +} + +// packageReleaseArchiveWith builds a release archive carrying the given members +// at its root, so a test can express a case as "an archive containing X". +func packageReleaseArchiveWith(t *testing.T, members []releaseMember) []byte { t.Helper() var buf bytes.Buffer if runtime.GOOS == "windows" { zw := zip.NewWriter(&buf) - hdr := &zip.FileHeader{Name: binaryName, Method: zip.Deflate} - hdr.SetMode(0o755) - w, err := zw.CreateHeader(hdr) - require.NoError(t, err) - _, err = w.Write(binary) - require.NoError(t, err) + for _, m := range members { + hdr := &zip.FileHeader{Name: m.name, Method: zip.Deflate} + hdr.SetMode(m.mode) + w, err := zw.CreateHeader(hdr) + require.NoError(t, err) + _, err = w.Write(m.body) + require.NoError(t, err) + } require.NoError(t, zw.Close()) return buf.Bytes() } gw := gzip.NewWriter(&buf) tw := tar.NewWriter(gw) - require.NoError(t, tw.WriteHeader(&tar.Header{ - Name: binaryName, - Mode: 0o755, - Size: int64(len(binary)), - Typeflag: tar.TypeReg, - })) - _, err := tw.Write(binary) - require.NoError(t, err) + for _, m := range members { + require.NoError(t, tw.WriteHeader(&tar.Header{ + Name: m.name, + Mode: int64(m.mode), + Size: int64(len(m.body)), + Typeflag: tar.TypeReg, + })) + _, err := tw.Write(m.body) + require.NoError(t, err) + } require.NoError(t, tw.Close()) require.NoError(t, gw.Close()) return buf.Bytes() @@ -641,3 +671,208 @@ func TestUpdateBinaryMockGitHubMissingChecksums(t *testing.T) { require.NoError(t, err) assert.Empty(t, leftovers, "aborted update must not leave temp files behind") } + +// bundledSetMembers is the archive-root names of the set a bundling release +// ships on this platform: the multi-call extensions binary and the descriptions +// file. It is what the release stamps into the binary via ldflags. +func bundledSetMembers() (bundledBinary, descriptions string) { + bundledBinary = "bundled-extensions" + if runtime.GOOS == "windows" { + bundledBinary += ".exe" + } + return bundledBinary, "lstk-extensions.toml" +} + +// TestUpdateRepairsIncompleteBundledSet is the transition case end to end: a +// user whose old updater installed only the lstk binary is left current but +// without bundled extensions, and cannot wait for a newer release to get them. +// `lstk update` must notice the incomplete set, reinstall the same version, and +// land the missing members. +func TestUpdateRepairsIncompleteBundledSet(t *testing.T) { + t.Parallel() + ctx := testContext(t) + + binaryName := "lstk" + if runtime.GOOS == "windows" { + binaryName = "lstk.exe" + } + bundledBinary, descriptions := bundledSetMembers() + + // The installed binary is already the latest version, and was built by the + // bundling release, but its install directory holds nothing else, exactly + // as a pre-bundling updater would have left it. + installDir := t.TempDir() + installed := filepath.Join(installDir, binaryName) + buildLstkWithBundledSet(t, ctx, "0.0.2", bundledBinary+","+descriptions, installed) + + newBytes, err := os.ReadFile(installed) + require.NoError(t, err) + archive := packageReleaseArchiveWith(t, []releaseMember{ + {name: binaryName, body: newBytes, mode: 0o755}, + {name: bundledBinary, body: []byte("multi-call extensions binary"), mode: 0o755}, + {name: descriptions, body: []byte("deploy = \"Deploy your application\"\n"), mode: 0o644}, + }) + sum := sha256.Sum256(archive) + assetName := releaseAssetName("0.0.2") + srv := mockGitHubReleaseServer(t, "v0.0.2", map[string][]byte{ + "checksums.txt": []byte(fmt.Sprintf("%s %s\n", hex.EncodeToString(sum[:]), assetName)), + assetName: archive, + }) + + updateCmd := exec.CommandContext(ctx, installed, "update", "--non-interactive") + updateCmd.Env = mockGitHubEnv(t, srv) + out, err := updateCmd.CombinedOutput() + outStr := string(out) + require.NoError(t, err, "lstk update failed: %s", outStr) + requireExitCode(t, 0, err) + + assert.NotContains(t, outStr, "Already up to date", + "a current binary with an incomplete set must not short-circuit: %s", outStr) + assert.Contains(t, outStr, "Bundled extensions are missing", "should state the finding") + assert.Contains(t, outStr, "Reinstalling 0.0.2 to restore bundled extensions", + "the apply path should narrate the reinstall: %s", outStr) + + // The missing members are now installed where lstk resolves them. + body, err := os.ReadFile(filepath.Join(installDir, bundledBinary)) + require.NoError(t, err, "the multi-call binary should have been installed") + assert.Equal(t, "multi-call extensions binary", string(body)) + body, err = os.ReadFile(filepath.Join(installDir, descriptions)) + require.NoError(t, err, "the descriptions file should have been installed") + assert.Equal(t, "deploy = \"Deploy your application\"\n", string(body)) + + leftovers, err := filepath.Glob(filepath.Join(installDir, "*.lstk-new")) + require.NoError(t, err) + assert.Empty(t, leftovers, "a completed update leaves no staging files") +} + +// TestUpdateReportsUpToDateWhenBundledSetComplete is the other half of the +// repair gate: with the version unchanged and the set complete, the ordinary +// up-to-date path must stay exactly as cheap as it was. The mock release serves +// no checksums.txt and no archive, so any attempt to download would fail the +// update loudly rather than pass unnoticed. +func TestUpdateReportsUpToDateWhenBundledSetComplete(t *testing.T) { + t.Parallel() + ctx := testContext(t) + + binaryName := "lstk" + if runtime.GOOS == "windows" { + binaryName = "lstk.exe" + } + bundledBinary, descriptions := bundledSetMembers() + + installDir := t.TempDir() + installed := filepath.Join(installDir, binaryName) + buildLstkWithBundledSet(t, ctx, "0.0.2", bundledBinary+","+descriptions, installed) + require.NoError(t, os.WriteFile(filepath.Join(installDir, bundledBinary), []byte("multi-call"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(installDir, descriptions), []byte("deploy = \"Deploy\"\n"), 0o644)) + + srv := mockGitHubReleaseServer(t, "v0.0.2", map[string][]byte{}) + + updateCmd := exec.CommandContext(ctx, installed, "update", "--non-interactive") + updateCmd.Env = mockGitHubEnv(t, srv) + out, err := updateCmd.CombinedOutput() + outStr := string(out) + require.NoError(t, err, "lstk update should succeed without downloading anything: %s", outStr) + requireExitCode(t, 0, err) + assert.Contains(t, outStr, "Already up to date", "output was: %s", outStr) + assert.NotContains(t, outStr, "Downloading", "a complete set must not trigger a download: %s", outStr) +} + +// TestUpdateIgnoresBundledSetOnPreBundlingRelease pins the rollback and +// pre-bundling behavior: a binary that ships no bundle never probes the install +// directory, so its up-to-date path is unchanged even with nothing beside it. +func TestUpdateIgnoresBundledSetOnPreBundlingRelease(t *testing.T) { + t.Parallel() + ctx := testContext(t) + + binaryName := "lstk" + if runtime.GOOS == "windows" { + binaryName = "lstk.exe" + } + installDir := t.TempDir() + installed := filepath.Join(installDir, binaryName) + buildLstkWithVersion(t, ctx, "0.0.2", installed) + + srv := mockGitHubReleaseServer(t, "v0.0.2", map[string][]byte{}) + + updateCmd := exec.CommandContext(ctx, installed, "update", "--non-interactive") + updateCmd.Env = mockGitHubEnv(t, srv) + out, err := updateCmd.CombinedOutput() + outStr := string(out) + require.NoError(t, err, "lstk update failed: %s", outStr) + assert.Contains(t, outStr, "Already up to date", "output was: %s", outStr) + assert.NotContains(t, outStr, "Bundled extensions are missing", "output was: %s", outStr) +} + +// TestUpdateRepairFailsLoudlyWhenArchiveLacksMembers guards against the silent +// repair loop: the binary is stamped with a bundled set, the release archive +// for the same version does not carry it, and without a post-install check +// every `lstk update` would re-download, report success, and fix nothing, +// forever. The update must fail and name what the archive did not deliver. +func TestUpdateRepairFailsLoudlyWhenArchiveLacksMembers(t *testing.T) { + t.Parallel() + ctx := testContext(t) + + binaryName := "lstk" + if runtime.GOOS == "windows" { + binaryName = "lstk.exe" + } + bundledBinary, descriptions := bundledSetMembers() + + installDir := t.TempDir() + installed := filepath.Join(installDir, binaryName) + buildLstkWithBundledSet(t, ctx, "0.0.2", bundledBinary+","+descriptions, installed) + + // The archive carries only lstk: the stamp promises members the release + // does not deliver. + newBytes, err := os.ReadFile(installed) + require.NoError(t, err) + archive := packageReleaseArchive(t, binaryName, newBytes) + sum := sha256.Sum256(archive) + assetName := releaseAssetName("0.0.2") + srv := mockGitHubReleaseServer(t, "v0.0.2", map[string][]byte{ + "checksums.txt": []byte(fmt.Sprintf("%s %s\n", hex.EncodeToString(sum[:]), assetName)), + assetName: archive, + }) + + updateCmd := exec.CommandContext(ctx, installed, "update", "--non-interactive") + updateCmd.Env = mockGitHubEnv(t, srv) + out, err := updateCmd.CombinedOutput() + outStr := string(out) + require.Error(t, err, "a repair that restored nothing must not report success: %s", outStr) + requireExitCode(t, 1, err) + assert.Contains(t, outStr, "did not restore", "should say the repair failed: %s", outStr) + assert.Contains(t, outStr, bundledBinary, "should name the member the archive lacks: %s", outStr) + assert.NotContains(t, outStr, "Updated to", "must not claim success: %s", outStr) +} + +// TestUpdateUpToDateCleansStagingLeftovers: a repair interrupted between +// committing the members and committing lstk itself leaves a staging copy of +// lstk behind, and the next run reports up to date without ever staging. The +// up-to-date path must still tidy those leftovers instead of leaking a +// binary-sized file until the next release. +func TestUpdateUpToDateCleansStagingLeftovers(t *testing.T) { + t.Parallel() + ctx := testContext(t) + + binaryName := "lstk" + if runtime.GOOS == "windows" { + binaryName = "lstk.exe" + } + installDir := t.TempDir() + installed := filepath.Join(installDir, binaryName) + buildLstkWithVersion(t, ctx, "0.0.2", installed) + leftover := filepath.Join(installDir, binaryName+".lstk-new") + require.NoError(t, os.WriteFile(leftover, []byte("interrupted staging copy"), 0o755)) + + srv := mockGitHubReleaseServer(t, "v0.0.2", map[string][]byte{}) + + updateCmd := exec.CommandContext(ctx, installed, "update", "--non-interactive") + updateCmd.Env = mockGitHubEnv(t, srv) + out, err := updateCmd.CombinedOutput() + require.NoError(t, err, "lstk update failed: %s", string(out)) + assert.Contains(t, string(out), "Already up to date") + + _, statErr := os.Stat(leftover) + assert.True(t, os.IsNotExist(statErr), "the up-to-date path should clean staging leftovers") +}