diff --git a/internal/manage/upgrade.go b/internal/manage/upgrade.go index f4ea56a800..43df0f493c 100644 --- a/internal/manage/upgrade.go +++ b/internal/manage/upgrade.go @@ -12,7 +12,7 @@ import ( // runtime-upgrade replaces the relay runtime from a release archive whose commit is green on the // dev gate, after proving no relay attempt is still open. Every step's command, exit status and -// output head is left in W/record.json. +// output head is left in W/record.json, together with every reason that applied to the run. // upgradeRecordHead is how many bytes of a command's output the record keeps. const upgradeRecordHead = 2000 @@ -40,6 +40,10 @@ const ( upgradeReasonStopFailed = "service_stop_failed" upgradeReasonPostCheck = "postcheck_failed" upgradeReasonUpdateFailed = "update_failed" + // The post-check's own findings are named apart from upgradeReasonPostCheck, so the record + // says what was found rather than only that the post-check refused. + upgradeReasonConfigChanged = "config_changed" + upgradeReasonRuntimeMismatch = "runtime_mismatch" ) const ( @@ -75,15 +79,19 @@ type upgradeStepRecord struct { } type upgradeRecord struct { - ReleaseDir string `json:"release_dir"` - Issue string `json:"issue,omitempty"` - DryRun bool `json:"dry_run"` - StartedAt string `json:"started_at"` - Directory string `json:"directory"` - ExtractDir string `json:"extract_dir,omitempty"` - Outcome string `json:"outcome"` - Reason string `json:"reason,omitempty"` - Steps []upgradeStepRecord `json:"steps"` + ReleaseDir string `json:"release_dir"` + Issue string `json:"issue,omitempty"` + DryRun bool `json:"dry_run"` + StartedAt string `json:"started_at"` + Directory string `json:"directory"` + ExtractDir string `json:"extract_dir,omitempty"` + // StartFrom is the runtime directory the restart was called from, so the record says which + // executable the service came back on. + StartFrom string `json:"start_from,omitempty"` + Outcome string `json:"outcome"` + Reason string `json:"reason,omitempty"` + Reasons []string `json:"reasons,omitempty"` + Steps []upgradeStepRecord `json:"steps"` } var upgradeCommand = Command{ @@ -161,7 +169,26 @@ type upgradeRunState struct { state string started time.Time + // version is the cleaned version the unpacked crw printed, installed the runtime directory the + // update reported it produced, previous the runtime the pointer named before the stop, and + // startFrom the runtime the restart used. The verified archive's own digest is not kept: the + // sums step records it. + version string + installed string + previous string + startFrom string + // serviceUp reports whether a start attempt left the service up - its own start succeeded, or the + // service was already running. It is what lets the post-check tell the runtime the service is + // really on from the last runtime it merely tried. + serviceUp bool + // promoted reports whether the update put a runtime in service (the installer's OK or + // Incomplete), which is what the post-check compares the pointer and the version with. An + // update that did not land leaves the pointer on the runtime it replaced, which is a correct + // rollback rather than a mismatch. + promoted bool + beforeConfig string + reasons []string steps []upgradeStepRecord } @@ -183,7 +210,8 @@ func (r *upgradeRunState) run() int { return code } -// execute performs the nine steps in order. Each stop returns before the step after it. +// execute performs the steps in order. Each stop returns before the step after it; the steps +// after the update always run, because the service has to come back either way. func (r *upgradeRunState) execute() (int, string) { archive, code, reason := r.verifySums() if code != 0 { @@ -210,12 +238,29 @@ func (r *upgradeRunState) execute() (int, string) { updateCode, updateReason := r.stopAndUpdate() r.start() - postCode, postReason := r.postCheck() + post := r.postCheck() + return r.outcome(updateCode, updateReason, post) +} + +// outcome is the run's final status and reason, in the decided order: the post-check's own findings +// outrank a failed update, which in turn outranks its other findings. A configuration change comes +// first, then the pointer-and-runtime mismatch the post-check exists to catch; both are exit 4, so a +// failed update can never hide either. Every reason that applied is kept in the record, so a run +// that both failed to update and changed the configuration names both. +func (r *upgradeRunState) outcome(updateCode int, updateReason string, post upgradePostCheck) (int, string) { if updateCode != 0 { - return updateCode, updateReason + r.reasons = append(r.reasons, updateReason) } - if postCode != 0 { - return postCode, postReason + r.reasons = append(r.reasons, post.reasons...) + switch { + case post.configChanged: + return upgradeExitPostCheck, upgradeReasonConfigChanged + case post.mismatch: + return upgradeExitPostCheck, upgradeReasonRuntimeMismatch + case updateCode != 0: + return updateCode, updateReason + case post.code != 0: + return post.code, post.reason } return 0, "" } @@ -228,9 +273,17 @@ func (r *upgradeRunState) write(reason string) error { StartedAt: r.started.Format(time.RFC3339), Directory: r.dir, ExtractDir: r.extract, + StartFrom: r.startFrom, Outcome: "ok", Steps: r.steps, } + // Every failure names its reasons. A run that refused before the update has only the one + // reason it stopped on, so the list carries that; a run that got as far as the update names + // every reason that applied, in the order they were decided. + record.Reasons = r.reasons + if len(record.Reasons) == 0 && reason != "" { + record.Reasons = []string{reason} + } if reason != "" { record.Outcome = "failed" record.Reason = reason diff --git a/internal/manage/upgrade_review702_test.go b/internal/manage/upgrade_review702_test.go new file mode 100644 index 0000000000..4d0615393b --- /dev/null +++ b/internal/manage/upgrade_review702_test.go @@ -0,0 +1,490 @@ +package manage + +import ( + "bytes" + "os" + "path/filepath" + "slices" + "strings" + "testing" + "time" +) + +// The review-702 cases: the defects the post-merge review of PR #702 found in +// crw manage runtime-upgrade, one test per decided answer. + +// upgradeReview702Swapped is an archive whose crw is not the verified one: it records that it ran +// and answers with a version that resolves nowhere. It is what an adversary puts in the release +// directory in the moment between the checksum being verified and the bytes being used. +func upgradeReview702Swapped(t *testing.T, marker string) []byte { + t.Helper() + return upgradeTarGz(t, map[string]string{"crw": "#!/bin/sh\n" + + "printf 'swapped\\n' >> " + coreShellQuote(marker) + "\n" + + "printf '%s\\n' 'v0.4.0-9999-gdeadbeef'\nexit 0\n"}) +} + +// TestUpgradeReview702SwappedArchiveIsNotRun: the release directory's archive is replaced at the +// moment the adversary has the chance - as soon as the checksum has been verified, before the bytes +// are used. The replacement really takes (the release directory holds the swapped archive when the +// run goes on), and it still changes nothing: the run unpacks the archive whose digest it verified, +// and the crw of the swapped archive never runs. The baseline hashes the original archive and then +// copies that same original, so the same replacement between those two reads is what it unpacks and +// runs; here the run has already copied the archive it verified, and the seam's swap reaches only +// the release directory. +func TestUpgradeReview702SwappedArchiveIsNotRun(t *testing.T) { + marker := filepath.Join(t.TempDir(), "swapped-crw-ran") + swapped := upgradeReview702Swapped(t, marker) + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), pointer: true}) + verified, err := os.ReadFile(filepath.Join(h.release, upgradeArchiveName)) + if err != nil { + t.Fatal(err) + } + + replaced := false + old := upgradeSumsVerified + upgradeSumsVerified = func(releaseDir string) { + replaced = true + if err := os.WriteFile(filepath.Join(releaseDir, upgradeArchiveName), swapped, 0o600); err != nil { + t.Errorf("replace the release archive: %v", err) + } + } + t.Cleanup(func() { upgradeSumsVerified = old }) + + code := h.run("--release-dir", h.release, "--dry-run") + + if !replaced { + t.Fatal("the release archive was never replaced, so this case proves nothing") + } + onDisk, err := os.ReadFile(filepath.Join(h.release, upgradeArchiveName)) + if err != nil { + t.Fatal(err) + } + if bytes.Equal(onDisk, verified) { + t.Fatal("the release directory still holds the verified archive, so the replacement did not take") + } + if _, err := os.Stat(marker); err == nil { + t.Error("the crw of the archive that replaced the verified one ran") + } + if code != 0 { + t.Fatalf("exit %d, want 0; the record is %+v", code, h.recordOf(t)) + } + pinned, err := os.ReadFile(filepath.Join(h.recordOf(t).Directory, upgradeArchiveName)) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(pinned, verified) { + t.Error("the archive the run unpacked is not the archive whose checksum it verified") + } + if calls := strings.Join(h.ghCallLines(), " "); !strings.Contains(calls, "commits/eb2567df7") { + t.Errorf("the verified archive's version did not resolve through gh: %q", calls) + } +} + +// TestUpgradeReview702StartRunsWhenUpdateBreaksThePointer: an update that removes the owned pointer +// and fails must still leave the service running, started from the runtime the pointer named before +// the stop, and the record must say so. +func TestUpgradeReview702StartRunsWhenUpdateBreaksThePointer(t *testing.T) { + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), + pointer: true, installExit: 1, breakPointer: true}) + if code := h.run("--release-dir", h.release); code == 0 { + t.Fatal("a broken pointer reported success") + } + if !h.calledFrom(filepath.Join(h.previous, "bin", "codex-session-relay"), "service start") { + t.Errorf("the service was not started from the runtime the pointer named before the stop: %q", h.callLines()) + } + if got, _ := h.recordJSON(t)["start_from"].(string); got != h.previous { + t.Errorf("the record names %q as what the service was started from, want %q", got, h.previous) + } +} + +// TestUpgradeReview702ConfigChangeWinsOverUpdateFailure: a configuration change the post-check +// found outranks a failed update, and the record keeps both reasons. +func TestUpgradeReview702ConfigChangeWinsOverUpdateFailure(t *testing.T) { + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), + pointer: true, installExit: 1, mutateConfig: true}) + if code := h.run("--release-dir", h.release); code != upgradeExitPostCheck { + t.Fatalf("exit %d, want %d; the record is %+v", code, upgradeExitPostCheck, h.recordOf(t)) + } + if got := h.recordOf(t).Reason; got != upgradeReasonConfigChanged { + t.Errorf("reason %q, want %q", got, upgradeReasonConfigChanged) + } + reasons := upgradeReview702Reasons(t, h) + for _, want := range []string{upgradeReasonUpdateFailed, upgradeReasonConfigChanged} { + if !slices.Contains(reasons, want) { + t.Errorf("the record does not name %q: %v", want, reasons) + } + } +} + +// upgradeReview702Reasons is the reasons the record names, read from the record as written. +func upgradeReview702Reasons(t *testing.T, h *upgradeEnv) []string { + t.Helper() + var out []string + values, _ := h.recordJSON(t)["reasons"].([]any) + for _, value := range values { + if text, ok := value.(string); ok { + out = append(out, text) + } + } + return out +} + +// TestUpgradeReview702StalePointerIsAMismatch: the post-check passes only when the pointer names +// the runtime the update reported it installed and that runtime reports the archive's version. +func TestUpgradeReview702StalePointerIsAMismatch(t *testing.T) { + t.Run("the pointer still names the previous runtime", func(t *testing.T) { + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), + pointer: true, produceRuntime: true}) + if code := h.run("--release-dir", h.release); code != upgradeExitPostCheck { + t.Fatalf("exit %d, want %d; the record is %+v", code, upgradeExitPostCheck, h.recordOf(t)) + } + if got := h.recordOf(t).Reason; got != upgradeReasonRuntimeMismatch { + t.Errorf("reason %q, want %q", got, upgradeReasonRuntimeMismatch) + } + if target, err := h.pointerTarget(); err != nil || target != h.previous { + t.Errorf("the pointer was moved to %q (%v), want it left at %q", target, err, h.previous) + } + }) + t.Run("the update names no runtime", func(t *testing.T) { + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), pointer: true}) + if code := h.run("--release-dir", h.release); code != upgradeExitPostCheck { + t.Fatalf("exit %d, want %d; the record is %+v", code, upgradeExitPostCheck, h.recordOf(t)) + } + if got := h.recordOf(t).Reason; got != upgradeReasonRuntimeMismatch { + t.Errorf("reason %q, want %q", got, upgradeReasonRuntimeMismatch) + } + }) + t.Run("the installed runtime reports another version", func(t *testing.T) { + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), pointer: true, + produceRuntime: true, pointAtIt: true, installedVersion: "v0.4.0-4633-gdeadbeef"}) + if code := h.run("--release-dir", h.release); code != upgradeExitPostCheck { + t.Fatalf("exit %d, want %d; the record is %+v", code, upgradeExitPostCheck, h.recordOf(t)) + } + if got := h.recordOf(t).Reason; got != upgradeReasonRuntimeMismatch { + t.Errorf("reason %q, want %q", got, upgradeReasonRuntimeMismatch) + } + }) +} + +// TestUpgradeReview702SlowWorkerIsWaitedFor: a service that answers running but not yet matching is +// read again, and a worker that settles a little later is not treated as a failure. +func TestUpgradeReview702SlowWorkerIsWaitedFor(t *testing.T) { + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), pointer: true, + produceRuntime: true, pointAtIt: true, + statusAnswers: []string{upgradeStatusUnknown, upgradeStatusUnknown, upgradeStatusSame}}) + if code := h.run("--release-dir", h.release); code != 0 { + t.Fatalf("exit %d, want 0; the record is %+v", code, h.recordOf(t)) + } + if reads := upgradeReview702StatusReads(h); reads != 3 { + t.Errorf("the service status was read %d times, want 3", reads) + } +} + +// TestUpgradeReview702OpenAttemptsFromDoctor: the open attempt count is the relay doctor's +// contents.openAttempts, read from the runtime the pointer names, and contents that could not be +// read are refused rather than read as none. +func TestUpgradeReview702OpenAttemptsFromDoctor(t *testing.T) { + t.Run("two open attempts refuse", func(t *testing.T) { + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), + pointer: true, openAttempts: 2}) + if code := h.run("--release-dir", h.release, "--dry-run"); code != upgradeExitOpenAttempts { + t.Fatalf("exit %d, want %d; the record is %+v", code, upgradeExitOpenAttempts, h.recordOf(t)) + } + if got := h.recordOf(t).Reason; got != upgradeReasonOpenAttempts { + t.Errorf("reason %q, want %q", got, upgradeReasonOpenAttempts) + } + if !h.calledFrom(filepath.Join(h.previous, "bin", "codex-session-relay"), "doctor") { + t.Errorf("the runtime the pointer names was not asked for its doctor answer: %q", h.callLines()) + } + }) + t.Run("unreadable contents refuse", func(t *testing.T) { + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), + pointer: true, doctorUnavailable: true}) + if code := h.run("--release-dir", h.release, "--dry-run"); code != upgradeExitRefused { + t.Fatalf("exit %d, want %d; the record is %+v", code, upgradeExitRefused, h.recordOf(t)) + } + if got := h.recordOf(t).Reason; got != upgradeReasonStoreRead { + t.Errorf("reason %q, want %q", got, upgradeReasonStoreRead) + } + }) + t.Run("contents with no count refuse", func(t *testing.T) { + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), + pointer: true, doctorNoCount: true}) + if code := h.run("--release-dir", h.release, "--dry-run"); code != upgradeExitRefused { + t.Fatalf("exit %d, want %d; the record is %+v", code, upgradeExitRefused, h.recordOf(t)) + } + if got := h.recordOf(t).Reason; got != upgradeReasonStoreRead { + t.Errorf("reason %q, want %q", got, upgradeReasonStoreRead) + } + }) +} + +// upgradeReview702StatusReads is how many times the run read the service status. +func upgradeReview702StatusReads(h *upgradeEnv) int { + reads := 0 + for _, line := range h.callArgs() { + if strings.Contains(line, "service status") { + reads++ + } + } + return reads +} + +// TestUpgradeReview702RestartsFromThePreviousRuntimeWhenTheNewOneWillNotStart: the pointer can +// resolve to a directory the service cannot be started from (a runtime whose relay executable is +// missing, say). The service is stopped by then, so the restart falls back to the runtime the +// pointer named before the stop rather than leaving the relay down, and the record names the +// runtime the service came back on. The recovery is kept, but the run must not report success: the +// runtime the update installed is not the one in service, so the pointer and the running code +// disagree, which is a mismatch. +func TestUpgradeReview702RestartsFromThePreviousRuntimeWhenTheNewOneWillNotStart(t *testing.T) { + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), pointer: true, + produceRuntime: true, pointAtIt: true, breakInstalledStart: true}) + if code := h.run("--release-dir", h.release); code != upgradeExitPostCheck { + t.Fatalf("exit %d, want %d; the installed runtime is not in service: %+v", code, upgradeExitPostCheck, h.recordOf(t)) + } + if got := h.recordOf(t).Reason; got != upgradeReasonRuntimeMismatch { + t.Errorf("reason %q, want %q", got, upgradeReasonRuntimeMismatch) + } + if !h.calledFrom(filepath.Join(h.installed, "bin", "codex-session-relay"), "service start") { + t.Errorf("the pointer's runtime was not tried: %q", h.callLines()) + } + if !h.calledFrom(filepath.Join(h.previous, "bin", "codex-session-relay"), "service start") { + t.Errorf("the previous runtime was not tried after the pointer's runtime failed to start: %q", h.callLines()) + } + if got := h.recordOf(t).StartFrom; got != h.previous { + t.Errorf("the record says the service came back on %q, want the previous runtime %q", got, h.previous) + } +} + +// TestUpgradeReview702ConfigDeletionWinsOverUpdateFailure: the snapshot only proceeded when it read +// a configuration file, so a file that is gone afterwards is a change the run can state exactly. It +// outranks the update failure, and the record names both reasons. +func TestUpgradeReview702ConfigDeletionWinsOverUpdateFailure(t *testing.T) { + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), pointer: true, + installExit: 1, deleteConfigAfter: true}) + if code := h.run("--release-dir", h.release); code != upgradeExitPostCheck { + t.Fatalf("exit %d, want %d; the record is %+v", code, upgradeExitPostCheck, h.recordOf(t)) + } + if got := h.recordOf(t).Reason; got != upgradeReasonConfigChanged { + t.Errorf("reason %q, want %q", got, upgradeReasonConfigChanged) + } + reasons := upgradeReview702Reasons(t, h) + for _, want := range []string{upgradeReasonUpdateFailed, upgradeReasonConfigChanged} { + if !slices.Contains(reasons, want) { + t.Errorf("the record does not name %q: %v", want, reasons) + } + } +} + +// TestUpgradeReview702UnreadableConfigIsNotAChange: a configuration file that exists but cannot be +// read is not a configuration change: the run cannot tell whether it changed, so it is a post-check +// finding of its own. An update failure keeps its own exit code, and the record must not claim +// config_changed. +func TestUpgradeReview702UnreadableConfigIsNotAChange(t *testing.T) { + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), pointer: true, + installExit: 1, unreadableConfigAfter: true}) + if code := h.run("--release-dir", h.release); code != upgradeExitUpdateFailed { + t.Fatalf("exit %d, want %d; the record is %+v", code, upgradeExitUpdateFailed, h.recordOf(t)) + } + if got := h.recordOf(t).Reason; got != upgradeReasonUpdateFailed { + t.Errorf("reason %q, want %q", got, upgradeReasonUpdateFailed) + } + if slices.Contains(h.recordOf(t).Reasons, upgradeReasonConfigChanged) { + t.Errorf("an unreadable configuration was recorded as a change: %v", h.recordOf(t).Reasons) + } +} + +// TestUpgradeReview702PreUpdateRefusalNamesItsReason: a run that refuses before the update still +// names its reason in the record's reasons list, not only in the single reason field. +func TestUpgradeReview702PreUpdateRefusalNamesItsReason(t *testing.T) { + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), pointer: true}) + if err := os.WriteFile(filepath.Join(h.release, upgradeSumsName), []byte("deadbeef "+upgradeArchiveName+"\n"), 0o600); err != nil { + t.Fatal(err) + } + if code := h.run("--release-dir", h.release); code != upgradeExitRefused { + t.Fatalf("exit %d, want %d", code, upgradeExitRefused) + } + if got := h.recordOf(t).Reasons; !slices.Contains(got, upgradeReasonSumsFailed) { + t.Errorf("the record's reasons are %v, want them to name %s", got, upgradeReasonSumsFailed) + } +} + +// TestUpgradeReview702FailedUpdateIsNotAVersionMismatch: when the update did not land, the pointer +// correctly stays on the runtime it replaced, whose version is not the archive's. That is a +// rollback, not a mismatch, so the record must not claim runtime_mismatch. +func TestUpgradeReview702FailedUpdateIsNotAVersionMismatch(t *testing.T) { + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), pointer: true, + installExit: 1, previousVersion: "v0.4.0-4633-gaaaaaaaa1"}) + if code := h.run("--release-dir", h.release); code != upgradeExitUpdateFailed { + t.Fatalf("exit %d, want %d; the record is %+v", code, upgradeExitUpdateFailed, h.recordOf(t)) + } + // The pointer still names the runtime the failed update replaced, which reports the previous + // version. That is the rollback working, not a mismatch. + if target, err := h.pointerTarget(); err != nil || target != h.previous { + t.Errorf("the pointer names %q (%v), want the previous runtime %q", target, err, h.previous) + } + if slices.Contains(h.recordOf(t).Reasons, upgradeReasonRuntimeMismatch) { + t.Errorf("a correct rollback was recorded as a version mismatch: %v", h.recordOf(t).Reasons) + } +} + +// TestUpgradeReview702IncompletePromotionIsChecked: the installer's exit 3 is a promotion whose +// claim did not settle, so the runtime is in service and the pointer must name it. An update that +// ends that way still has to fail the run, and a pointer that does not name what it promoted is +// still a mismatch. +func TestUpgradeReview702IncompletePromotionIsChecked(t *testing.T) { + t.Run("the pointer names what it promoted", func(t *testing.T) { + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), pointer: true, + produceRuntime: true, pointAtIt: true, installExit: 3}) + if code := h.run("--release-dir", h.release); code != upgradeExitUpdateFailed { + t.Fatalf("exit %d, want %d; the record is %+v", code, upgradeExitUpdateFailed, h.recordOf(t)) + } + if got := h.recordOf(t).Reasons; slices.Contains(got, upgradeReasonRuntimeMismatch) { + t.Errorf("the pointer names the runtime the update promoted, so there is no mismatch: %v", got) + } + }) + t.Run("the pointer names another runtime", func(t *testing.T) { + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), pointer: true, + produceRuntime: true, installExit: 3}) + if code := h.run("--release-dir", h.release); code != upgradeExitPostCheck { + t.Fatalf("exit %d, want %d; the record is %+v", code, upgradeExitPostCheck, h.recordOf(t)) + } + if got := h.recordOf(t).Reason; got != upgradeReasonRuntimeMismatch { + t.Errorf("reason %q, want %q", got, upgradeReasonRuntimeMismatch) + } + if got := h.recordOf(t).Reasons; !slices.Contains(got, upgradeReasonRuntimeMismatch) { + t.Errorf("the pointer does not name the runtime the update promoted, so it is a mismatch: %v", got) + } + }) +} + +// TestUpgradeReview702ServiceAlreadyRunningIsNotAFailure: the runtime the update installed is +// already up when the restart runs, as it is when something started it between the stop and the +// restart. Its start answers already_running rather than launching a second one; that is the runtime +// the service is on, not a failed launch to fall back from, so the run must not report the healthy +// new runtime as a mismatch. +func TestUpgradeReview702ServiceAlreadyRunningIsNotAFailure(t *testing.T) { + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), pointer: true, + produceRuntime: true, pointAtIt: true, installedStartAlreadyRunning: true}) + if code := h.run("--release-dir", h.release); code != 0 { + t.Fatalf("exit %d, want 0; the service is up on the runtime the update installed: %+v", code, h.recordOf(t)) + } + if got := h.recordOf(t).Reasons; slices.Contains(got, upgradeReasonRuntimeMismatch) { + t.Errorf("a service already running on the installed runtime was recorded as a mismatch: %v", got) + } + if got := h.recordOf(t).StartFrom; got != h.installed { + t.Errorf("the record says the service is on %q, want the installed runtime %q", got, h.installed) + } +} + +// TestUpgradeReview702PointerMovedToAThirdRuntimeIsAMismatch: an update that failed is a rollback +// only when the pointer still names the runtime it replaced. A pointer left on some other runtime +// was changed by something other than this run, and nothing here can show what is in service, so it +// is a mismatch rather than a rollback. +func TestUpgradeReview702PointerMovedToAThirdRuntimeIsAMismatch(t *testing.T) { + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), pointer: true, + installExit: 1, pointAtOther: true}) + if code := h.run("--release-dir", h.release); code != upgradeExitPostCheck { + t.Fatalf("exit %d, want %d; the record is %+v", code, upgradeExitPostCheck, h.recordOf(t)) + } + if got := h.recordOf(t).Reason; got != upgradeReasonRuntimeMismatch { + t.Errorf("reason %q, want %q", got, upgradeReasonRuntimeMismatch) + } + if got := h.recordOf(t).Reasons; !slices.Contains(got, upgradeReasonRuntimeMismatch) { + t.Errorf("a pointer on a third runtime was accepted as a rollback: %v", got) + } +} + +// TestUpgradeReview702PointerBrokenDuringTheWaitIsAMismatch: the install target is verified before +// the service wait and again after it. The wait can run for a minute, and another process on the +// host can move or remove the pointer while this run waits; the last status answer alone must not +// turn that into a success. +func TestUpgradeReview702PointerBrokenDuringTheWaitIsAMismatch(t *testing.T) { + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), pointer: true, + produceRuntime: true, pointAtIt: true, breakPointerDuringWait: true, + statusAnswers: []string{upgradeStatusUnknown, upgradeStatusSame}}) + if code := h.run("--release-dir", h.release); code != upgradeExitPostCheck { + t.Fatalf("exit %d, want %d; the pointer was removed while the run waited: %+v", code, upgradeExitPostCheck, h.recordOf(t)) + } + if got := h.recordOf(t).Reason; got != upgradeReasonRuntimeMismatch { + t.Errorf("reason %q, want %q", got, upgradeReasonRuntimeMismatch) + } +} + +// TestUpgradeReview702ServedStoreOpenAttempts: the doctor says which store the relay service serves +// when discovery selected another directory. The selected directory's contents are not the served +// store's, so the served store must be asked for its own answer: a served store with open attempts +// refuses, and one that cannot be read is refused too rather than read as none. +func TestUpgradeReview702ServedStoreOpenAttempts(t *testing.T) { + t.Run("the served store refuses", func(t *testing.T) { + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), + pointer: true, doctorServesAnotherStore: true, servedStoreOpenAttempts: 2}) + if code := h.run("--release-dir", h.release, "--dry-run"); code != upgradeExitOpenAttempts { + t.Fatalf("exit %d, want %d; the record is %+v", code, upgradeExitOpenAttempts, h.recordOf(t)) + } + if got := h.recordOf(t).Reason; got != upgradeReasonOpenAttempts { + t.Errorf("reason %q, want %q", got, upgradeReasonOpenAttempts) + } + }) + t.Run("the served store is read, not the selected one", func(t *testing.T) { + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), + pointer: true, doctorServesAnotherStore: true, servedStoreOpenAttempts: 0}) + if code := h.run("--release-dir", h.release, "--dry-run"); code != 0 { + t.Fatalf("exit %d, want 0; the served store has nothing open: %+v", code, h.recordOf(t)) + } + if !h.calledFrom(filepath.Join(h.previous, "bin", "codex-session-relay"), h.state+"-served") { + t.Errorf("the served store was not asked for its own answer: %q", h.callLines()) + } + }) +} + +// TestUpgradeReview702OversizedArchiveIsRefusedBeforeItIsCopied: the pre-verification copy reads at +// most the installer's own input bound. A release directory holding something larger than the +// archive is refused, and the partial copy is removed, so a file that is not the archive cannot +// fill the state disk the live relay shares before the checksum even runs. +func TestUpgradeReview702OversizedArchiveIsRefusedBeforeItIsCopied(t *testing.T) { + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), pointer: true}) + old := upgradeMaxArchiveBytes + upgradeMaxArchiveBytes = 8 + t.Cleanup(func() { upgradeMaxArchiveBytes = old }) + if code := h.run("--release-dir", h.release, "--dry-run"); code != upgradeExitRefused { + t.Fatalf("exit %d, want %d; the record is %+v", code, upgradeExitRefused, h.recordOf(t)) + } + if got := h.recordOf(t).Reason; got != upgradeReasonSumsFailed { + t.Errorf("reason %q, want %q", got, upgradeReasonSumsFailed) + } + record := h.recordOf(t) + for _, name := range []string{upgradeArchiveName, upgradeSumsName} { + if _, err := os.Stat(filepath.Join(record.Directory, name)); err == nil { + t.Errorf("the refused copy left %s behind", name) + } + } +} + +// TestUpgradeReview702ServiceWaitBudgetIsRecorded: a service that never reports itself running and +// matching is a post-check failure after the real budget, and the wait records the last answer it +// read. The budget is shrunk here so the test spends it rather than the wall clock's 60 seconds; the +// product's own budget is the default the run reads. +func TestUpgradeReview702ServiceWaitBudgetIsRecorded(t *testing.T) { + oldBudget, oldInterval := upgradeServiceBudget, upgradeServiceInterval + upgradeServiceBudget, upgradeServiceInterval = 20*time.Millisecond, 5*time.Millisecond + t.Cleanup(func() { upgradeServiceBudget, upgradeServiceInterval = oldBudget, oldInterval }) + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), pointer: true, + produceRuntime: true, pointAtIt: true, statusAnswers: []string{upgradeStatusUnknown}}) + if code := h.run("--release-dir", h.release); code != upgradeExitPostCheck { + t.Fatalf("exit %d, want %d; the record is %+v", code, upgradeExitPostCheck, h.recordOf(t)) + } + if got := h.recordOf(t).Reason; got != upgradeReasonPostCheck { + t.Errorf("reason %q, want %q", got, upgradeReasonPostCheck) + } + recorded := "" + for _, step := range h.recordOf(t).Steps { + if step.Step == upgradeStepPostCheck && strings.Contains(step.Output, "matchesRunning") { + recorded = step.Output + } + } + if !strings.Contains(recorded, "unknown") { + t.Errorf("the wait did not record its last status answer: %q", recorded) + } +} diff --git a/internal/manage/upgrade_steps.go b/internal/manage/upgrade_steps.go index d3f04bde5a..16c593e1f1 100644 --- a/internal/manage/upgrade_steps.go +++ b/internal/manage/upgrade_steps.go @@ -14,21 +14,46 @@ import ( "os" "os/exec" "path/filepath" + "slices" "strings" "time" - "github.com/thisisjun786/codex-relay-workflow/internal/relay/store" + "github.com/thisisjun786/codex-relay-workflow/internal/runtime/install" ) const ( - upgradeSumsName = "SHA256SUMS" - upgradeArchiveGlob = "crw_*_linux_amd64.tar.gz" - upgradeRuntimeDir = ".local/share/crw-runtime" - upgradeStoreTimeout = 5 * time.Second + upgradeSumsName = "SHA256SUMS" + upgradeArchiveGlob = "crw_*_linux_amd64.tar.gz" + upgradeRuntimeDir = ".local/share/crw-runtime" // upgradeCommandTimeout bounds one forge call; upgradeInstallTimeout one lifecycle command, // which exercises the bridge and may copy a large relay state. upgradeCommandTimeout = 60 * time.Second upgradeInstallTimeout = 30 * time.Minute + // upgradeInstallOK and upgradeInstallIncomplete are the installer's own statuses for an update + // that promoted the runtime: OK, and Incomplete (the candidate is selected and the owned + // pointer names it, and only the claim that records it did not settle). Neither is this + // command's status space: an update that ends Incomplete is reported as failed here, but the + // runtime is in service, so the post-check still compares the pointer with it. + upgradeInstallOK = 0 + upgradeInstallIncomplete = 3 + // upgradeSumsBytes bounds the SHA256SUMS copy: the file is a list of digests, so a source far + // larger than that is not one. + upgradeSumsBytes = 1 << 20 +) + +// upgradeMaxArchiveBytes bounds what the pre-verification archive copy reads. It matches the +// installer's own input limit, so a release directory holding something that is not the archive is +// refused before it can fill the state disk the live relay shares. upgradeCopy removes its partial +// file on any failure, so a refused or failed copy leaves nothing behind. A variable so a test can +// reach the bound without writing 256 MiB. +var upgradeMaxArchiveBytes int64 = install.MaxArchiveBytes + +// upgradeServiceInterval and upgradeServiceBudget are how often, and for how long, the post-check +// reads the service status again after starting it. Variables so a test can pin the wait's timing +// rather than spend the real budget. +var ( + upgradeServiceInterval = 2 * time.Second + upgradeServiceBudget = 60 * time.Second ) // upgradeRunCommand runs exe and returns its stdout, stderr and exit status. @@ -72,48 +97,65 @@ func upgradeOutputHead(text string) string { return text[:upgradeRecordHead] } -// verifySums is step 1: find the archive and verify it against the SHA256SUMS beside it. +// verifySums is step 1: copy the release archive and its SHA256SUMS into the run directory, then +// verify the copy against the copied sums. The originals in the release directory are opened once, +// by the copy, and never again: the bytes that were checked are the bytes every later step reads, +// so replacing an original after the check cannot change what is unpacked and run. func (r *upgradeRunState) verifySums() (string, int, string) { matches, err := filepath.Glob(filepath.Join(r.opts.ReleaseDir, upgradeArchiveGlob)) if err != nil || len(matches) == 0 { r.note(upgradeStepSums, nil, 1, "", fmt.Errorf("no %s in %s", upgradeArchiveGlob, r.opts.ReleaseDir)) return "", upgradeExitRefused, upgradeReasonSumsFailed } - archive := matches[0] - sums, err := os.ReadFile(filepath.Join(r.opts.ReleaseDir, upgradeSumsName)) + name := filepath.Base(matches[0]) + pinned := filepath.Join(r.dir, name) + if err := upgradeCopy(matches[0], pinned, upgradeMaxArchiveBytes); err != nil { + r.note(upgradeStepSums, nil, 1, "", err) + return "", upgradeExitRefused, upgradeReasonSumsFailed + } + if err := upgradeCopy(filepath.Join(r.opts.ReleaseDir, upgradeSumsName), filepath.Join(r.dir, upgradeSumsName), upgradeSumsBytes); err != nil { + r.note(upgradeStepSums, nil, 1, "", err) + return "", upgradeExitRefused, upgradeReasonSumsFailed + } + sums, err := os.ReadFile(filepath.Join(r.dir, upgradeSumsName)) if err != nil { r.note(upgradeStepSums, nil, 1, "", err) return "", upgradeExitRefused, upgradeReasonSumsFailed } - want := upgradeSumsFor(string(sums), filepath.Base(archive)) + want := upgradeSumsFor(string(sums), name) if want == "" { - r.note(upgradeStepSums, nil, 1, "", fmt.Errorf("%s names no digest for %s", upgradeSumsName, filepath.Base(archive))) + r.note(upgradeStepSums, nil, 1, "", fmt.Errorf("%s names no digest for %s", upgradeSumsName, name)) return "", upgradeExitRefused, upgradeReasonSumsFailed } - got, err := upgradeFileDigest(archive) + got, err := upgradeFileDigest(pinned) if err != nil { r.note(upgradeStepSums, nil, 1, "", err) return "", upgradeExitRefused, upgradeReasonSumsFailed } if !strings.EqualFold(got, want) { - r.note(upgradeStepSums, nil, 1, "", fmt.Errorf("%s does not match %s", filepath.Base(archive), upgradeSumsName)) + r.note(upgradeStepSums, nil, 1, "", fmt.Errorf("%s does not match %s", name, upgradeSumsName)) return "", upgradeExitRefused, upgradeReasonSumsFailed } - r.note(upgradeStepSums, nil, 0, want+" "+filepath.Base(archive), nil) - // Pin the verified bytes so the archive cannot be swapped after the check; the installer - // rechecks against the SHA256SUMS, so that file is pinned too. - pinned := filepath.Join(r.dir, filepath.Base(archive)) - for _, src := range []string{archive, filepath.Join(r.opts.ReleaseDir, upgradeSumsName)} { - if err := upgradeCopy(src, filepath.Join(r.dir, filepath.Base(src))); err != nil { - r.note(upgradeStepSums, nil, 1, "", err) - return "", upgradeExitRefused, upgradeReasonSumsFailed - } + r.note(upgradeStepSums, nil, 0, want+" "+name, nil) + if upgradeSumsVerified != nil { + upgradeSumsVerified(r.opts.ReleaseDir) } return pinned, 0, "" } -// upgradeCopy copies src to dst, so a later reader cannot be given different bytes. -func upgradeCopy(src, dst string) error { +// upgradeSumsVerified runs at the one moment the release directory's originals could still change +// what runs: after the copied archive's digest has been compared with the copied SHA256SUMS, and +// before anything is unpacked. It is nil everywhere but a test, which uses it to replace the +// release directory's archive there. The run never opens that archive again, so what replaced it +// cannot be what runs; the baseline hashed the original and then copied it, and the same +// replacement between those two reads changed the bytes that were executed. +var upgradeSumsVerified func(releaseDir string) + +// upgradeCopy copies src to dst, so a later reader cannot be given different bytes. At most limit +// bytes are read: a source over it is refused rather than copied, so a release directory holding +// something that is not the archive cannot fill the state disk before verification. A copy that +// fails for any reason leaves no partial file behind. +func upgradeCopy(src, dst string, limit int64) error { in, err := os.Open(src) if err != nil { return err @@ -123,11 +165,19 @@ func upgradeCopy(src, dst string) error { if err != nil { return err } - if _, err := io.Copy(out, in); err != nil { - out.Close() + written, err := io.Copy(out, io.LimitReader(in, limit+1)) + if cerr := out.Close(); err == nil { + err = cerr + } + if err == nil && written > limit { + err = fmt.Errorf("%s is larger than %d bytes", filepath.Base(src), limit) + } + if err != nil { + // The partial copy is this call's own file; remove it so a refusal leaves nothing behind. + os.Remove(dst) return err } - return out.Close() + return nil } // upgradeSumsFor is the digest SHA256SUMS lists for name, or "" when it lists none. @@ -154,7 +204,9 @@ func upgradeFileDigest(path string) (string, error) { return hex.EncodeToString(h.Sum(nil)), nil } -// extractAndResolve is step 2: unpack the archive and resolve the version to a full commit SHA. +// extractAndResolve is step 2: unpack the verified copy and resolve the version to a full commit +// SHA. The unpacked crw's version is kept, because the post-check compares the installed runtime's +// version with the one this archive carried. func (r *upgradeRunState) extractAndResolve() (int, string) { r.extract = filepath.Join(r.dir, "extract") if err := os.MkdirAll(r.extract, 0o700); err != nil { @@ -168,6 +220,7 @@ func (r *upgradeRunState) extractAndResolve() (int, string) { if err != nil || code != 0 { return upgradeExitRefused, upgradeReasonExtractFailed } + r.version = strings.TrimSpace(version) for _, ref := range upgradeCommitRefs(version) { out, code, err := r.command(r.ctx, upgradeCommandTimeout, upgradeStepCommit, "gh", "api", "repos/"+r.cfg.Repository+"/commits/"+ref) if err != nil || code != 0 { @@ -228,40 +281,105 @@ func (r *upgradeRunState) checkDevGate() (int, string) { return r.refuse(upgradeStepDevGate, upgradeReasonDevGate, out, fmt.Errorf("no dev-gate check run concluded success")) } -// checkOpenAttempts is step 4: read the store read-only and refuse while any attempt is unsettled. +// upgradeDoctorAnswer is the part of the relay doctor's answer this command reads: where the store +// is, and how many attempts the service that holds it still has open. +type upgradeDoctorAnswer struct { + StateSelection struct { + Path string `json:"path"` + } `json:"stateSelection"` + Contents struct { + Available bool `json:"available"` + OpenAttempts *int64 `json:"openAttempts"` + } `json:"contents"` + // ServiceStore is present only when the relay service registered for this socket serves another + // directory than the one discovery selected: the contents above then belong to the selected + // directory, not to the store the service reads and writes, so they are not the served store's + // open attempts. + ServiceStore *struct { + StateDirectory string `json:"stateDirectory"` + SocketPath string `json:"socketPath"` + } `json:"serviceStore"` +} + +// upgradeParseDoctorAnswer reads the doctor's answer; an answer that is not JSON is an error, so a +// reader never mistakes an unreadable store for an empty one. +func upgradeParseDoctorAnswer(out string) (upgradeDoctorAnswer, error) { + var answer upgradeDoctorAnswer + if err := json.Unmarshal([]byte(out), &answer); err != nil { + return answer, fmt.Errorf("the relay doctor answer: %w", err) + } + return answer, nil +} + +// checkOpenAttempts is step 4: ask the running runtime's own relay for its doctor answer and refuse +// while any attempt is still open. The count is the doctor's contents.openAttempts alone; contents +// that are unavailable, or that carry no count, are refused rather than read as an absence, so a +// store nobody could read never looks like a store with nothing open. When the doctor says the +// service serves another directory than the one discovery selected, that other store is the one +// whose attempts matter, so it is asked for its own answer; a served store whose answer cannot be +// read is refused rather than passed over. func (r *upgradeRunState) checkOpenAttempts() (int, string) { - state, err := r.relayState() + pointer, err := upgradePointerTarget(r.e) if err != nil { - return r.refuse(upgradeStepAttempts, upgradeReasonStoreRead, "", err) + // There is no running runtime to ask, which is the pointer's own refusal: the same reason the + // snapshot reported for this host before this step moved ahead of it. + return r.refuse(upgradeStepAttempts, upgradeReasonPointer, "", err) + } + relay := filepath.Join(pointer, "bin", "codex-session-relay") + state := r.cfg.Relay.State + socket := r.cfg.Relay.Socket + answer, out, code, err := r.doctorAnswer(relay, state, socket) + if err != nil || code != 0 { + return upgradeExitRefused, upgradeReasonStoreRead } - r.state = state - ctx, cancel := context.WithTimeout(r.ctx, upgradeStoreTimeout) - defer cancel() - read, err := store.OpenReadOnly(ctx, filepath.Join(state, "relay.sqlite3"), upgradeStoreTimeout) + if served := answer.ServiceStore; served != nil && served.StateDirectory != "" { + // The doctor adds serviceStore only when the directory it read is not the one the service + // serves, so these contents are another store's. Ask the served store for its own answer; + // anything unreadable there is refused rather than read as none. + servedSocket := served.SocketPath + if servedSocket == "" { + servedSocket = socket + } + answer, out, code, err = r.doctorAnswer(relay, served.StateDirectory, servedSocket) + if err != nil || code != 0 { + return upgradeExitRefused, upgradeReasonStoreRead + } + state = served.StateDirectory + } + answer, err = upgradeParseDoctorAnswer(out) if err != nil { - return r.refuse(upgradeStepAttempts, upgradeReasonStoreRead, "", err) + return r.refuse(upgradeStepAttempts, upgradeReasonStoreRead, out, err) } - defer read.Close() - var open int - query := "select count(*) from attempts where internal_state != 'settled'" - if err := read.QueryRowContext(ctx, query).Scan(&open); err != nil { - r.note(upgradeStepAttempts, []string{query}, 1, "", err) - return upgradeExitRefused, upgradeReasonStoreRead + if state == "" { + state = answer.StateSelection.Path + } + if state == "" { + return r.refuse(upgradeStepAttempts, upgradeReasonStoreRead, out, errors.New("the doctor named no state directory")) + } + r.state = state + if !answer.Contents.Available || answer.Contents.OpenAttempts == nil { + return r.refuse(upgradeStepAttempts, upgradeReasonStoreRead, out, fmt.Errorf("the doctor's contents carry no open attempt count")) } - r.note(upgradeStepAttempts, []string{query}, 0, fmt.Sprintf("%d", open), nil) - if open != 0 { + if *answer.Contents.OpenAttempts != 0 { return upgradeExitOpenAttempts, upgradeReasonOpenAttempts } return 0, "" } -// relayState is the state directory the run reads and stops: configured, else the doctor's. -func (r *upgradeRunState) relayState() (string, error) { - if r.cfg.Relay.State != "" { - return r.cfg.Relay.State, nil +// doctorAnswer runs the relay's doctor for one state directory (or discovery when state is empty) +// and parses its answer, recording the call. +func (r *upgradeRunState) doctorAnswer(relay, state, socket string) (upgradeDoctorAnswer, string, int, error) { + args := []string{"--socket", socket} + if state != "" { + args = append([]string{"--state", state}, args...) + } + args = append(args, "doctor") + out, code, err := r.command(r.ctx, upgradeCommandTimeout, upgradeStepAttempts, relay, args...) + if err != nil || code != 0 { + return upgradeDoctorAnswer{}, out, code, err } - state, err := r.e.relayHelperDoctorState(r.ctx, r.cfg.Relay.Socket) - return state, err + answer, err := upgradeParseDoctorAnswer(out) + return answer, out, code, err } // snapshot is step 5: what the run must be able to compare afterwards. @@ -283,13 +401,15 @@ func (r *upgradeRunState) snapshot() (int, string) { } // stopAndUpdate is step 6: stop with the running executable, then install with the extracted one. -// It returns the status and reason to report: a stop that did not succeed stops the run before -// the runtime is replaced, and the two failures are named apart in the record. +// The runtime the pointer names is recorded before the stop, so the restart after it can fall back +// to the runtime the service was running. A stop that did not succeed stops the run before the +// runtime is replaced, and the two failures are named apart in the record. func (r *upgradeRunState) stopAndUpdate() (int, string) { pointer, err := upgradePointerTarget(r.e) if err != nil { return r.refuse(upgradeStepStop, upgradeReasonStopFailed, "", err) } + r.previous = pointer if _, code, err := r.command(r.ctx, upgradeInstallTimeout, upgradeStepStop, filepath.Join(pointer, "bin", "codex-session-relay"), "--state", r.state, "--socket", r.cfg.Relay.Socket, "service", "stop"); err != nil || code != 0 { @@ -300,57 +420,249 @@ func (r *upgradeRunState) stopAndUpdate() (int, string) { if r.opts.Issue != "" { args = append(args, "--issue", r.opts.Issue) } - _, code, err := r.command(r.ctx, upgradeInstallTimeout, upgradeStepUpdate, filepath.Join(r.extract, "crw"), args...) + out, code, err := r.command(r.ctx, upgradeInstallTimeout, upgradeStepUpdate, filepath.Join(r.extract, "crw"), args...) if err != nil { return upgradeExitUpdateFailed, upgradeReasonUpdateFailed } + if code == upgradeInstallOK || code == upgradeInstallIncomplete { + // The installer's Incomplete is a promotion whose claim did not settle: the candidate is + // selected and the owned pointer already names it, so the runtime is in service and the + // post-check compares the pointer and the version with it, even though this command reports + // the run as failed because the promotion did not finish. + r.promoted = true + r.installed = upgradeInstalledRuntime(out) + } if code != 0 { return upgradeExitUpdateFailed, upgradeReasonUpdateFailed } return 0, "" } -// start is step 7: start from the new pointer's executable, whether or not the update succeeded. +// upgradeInstalledRuntime is the runtime directory the update's own answer names, or "" when the +// answer names none. Nothing else can say where the update put the runtime, and the post-check +// refuses rather than comparing the pointer with a guess. +func upgradeInstalledRuntime(answer string) string { + var reported struct { + Environment string `json:"environment"` + } + if json.Unmarshal([]byte(answer), &reported) != nil { + return "" + } + return reported.Environment +} + +// start is step 7: start again, whatever the update did. The pointer's runtime is tried first when +// it still resolves; a pointer that does not resolve, or a start that does not succeed, falls back +// to the runtime the pointer named before the stop, which is what the service was running. A +// pointer left on an unusable runtime therefore cannot leave the relay down. Every attempt is +// recorded, and startFrom is the runtime the service is on: the one whose start succeeded, or the +// one whose start answered that the service was already running. A service already up holds the +// lock, so no other runtime can start either, and falling back would only name a runtime the +// service is not on. func (r *upgradeRunState) start() { - pointer, err := upgradePointerTarget(r.e) - if err != nil { + var candidates []string + if pointer, err := upgradePointerTarget(r.e); err == nil { + candidates = append(candidates, pointer) + } else if r.previous != "" { + r.note(upgradeStepStart, nil, 1, "", err) + } else { r.note(upgradeStepStart, nil, 1, "", err) return } - // The recovery step runs detached, so a cancellation after the stop cannot leave it down. - ctx, cancel := context.WithTimeout(context.WithoutCancel(r.ctx), upgradeInstallTimeout) - defer cancel() - _, _, _ = r.command(ctx, upgradeInstallTimeout, upgradeStepStart, filepath.Join(pointer, "bin", "codex-session-relay"), - "--state", r.state, "--socket", r.cfg.Relay.Socket, "service", "start") + if r.previous != "" && !slices.Contains(candidates, r.previous) { + candidates = append(candidates, r.previous) + } + for i, runtime := range candidates { + // The recovery step runs detached, so a cancellation after the stop cannot leave it down. + ctx, cancel := context.WithTimeout(context.WithoutCancel(r.ctx), upgradeInstallTimeout) + argv := []string{filepath.Join(runtime, "bin", "codex-session-relay"), + "--state", r.state, "--socket", r.cfg.Relay.Socket, "service", "start"} + out, stderr, code, err := upgradeRunCommand(ctx, argv[0], argv[1:]...) + cancel() + from := "the pointer" + if runtime == r.previous { + from = "the runtime the pointer named before the stop" + } + r.note(upgradeStepStart, argv, code, "started from "+from+": "+runtime+"\n"+out+stderr, err) + if err == nil && code == 0 { + r.startFrom = runtime + r.serviceUp = true + return + } + if err == nil && upgradeServiceAlreadyRunning(out) { + r.startFrom = runtime + r.serviceUp = true + return + } + if i+1 == len(candidates) { + // Nothing started the service, so startFrom stays empty: it names the runtime the service + // came back on, and no runtime did. The post-check reads the status through the pointer and + // reports what it finds; the last runtime tried is in the recorded step above. + r.note(upgradeStepStart, nil, 1, "", fmt.Errorf("no runtime started the service; the last attempt was %s", runtime)) + } + } } -// postCheck is step 8: the new pointer and version, the service running and matching, and an -// unchanged configuration file. -func (r *upgradeRunState) postCheck() (int, string) { - pointer, err := upgradePointerTarget(r.e) - if err != nil { - r.note(upgradeStepPostCheck, nil, 1, "", err) - return upgradeExitPostCheck, upgradeReasonPostCheck +// upgradeServiceAlreadyRunning reports whether a start answer says the service was already up. That +// is not a failed launch: the service holds the lock, so this is the runtime it is on, and the +// status the post-check reads next is what decides whether it is the runtime the update installed. +func upgradeServiceAlreadyRunning(out string) bool { + var answer struct { + Reason string `json:"reason"` } - version, code, err := r.command(r.ctx, upgradeCommandTimeout, upgradeStepPostCheck, filepath.Join(pointer, "bin", "crw"), "--version") - if err != nil || code != 0 || strings.TrimSpace(version) == "" { - return upgradeExitPostCheck, upgradeReasonPostCheck + if json.Unmarshal([]byte(out), &answer) != nil { + return false } - out, code, err := r.command(r.ctx, upgradeCommandTimeout, upgradeStepPostCheck, filepath.Join(pointer, "bin", "codex-session-relay"), - "--state", r.state, "--socket", r.cfg.Relay.Socket, "service", "status") - if err != nil || code != 0 { - return upgradeExitPostCheck, upgradeReasonPostCheck + return answer.Reason == "already_running" +} + +// upgradePostCheck is what step 8 established: the status and reason it reports, every reason it +// found, and whether the configuration file changed. +type upgradePostCheck struct { + code int + reason string + reasons []string + configChanged bool + // mismatch reports that the post-check found the pointer and the runtime in service disagreeing + // with the runtime the update installed. It is carried apart from the other findings because the + // decided order lets it outrank a failed update, which a service that merely came up slowly does + // not. + mismatch bool +} + +// postCheck is step 8: the pointer must name the runtime the update installed and that runtime's +// crw must report the version the archive carried, or - when the update did not put a runtime in +// service - the pointer must still name the runtime it replaced, which is the rollback. The service +// must come up running and matching on that runtime within the wait budget, and the configuration +// file must be unchanged. The install target is verified again after the wait: the wait can take a +// minute, and another process on the host can move the pointer while this run waits. +func (r *upgradeRunState) postCheck() upgradePostCheck { + var post upgradePostCheck + r.verifyInstallTarget(&post) + runtime := r.startFrom + if runtime == "" { + if pointer, err := upgradePointerTarget(r.e); err == nil { + runtime = pointer + } } - if !upgradeServiceRunning(out) { - r.note(upgradeStepPostCheck, nil, 1, out, fmt.Errorf("the service is not running and matching")) - return upgradeExitPostCheck, upgradeReasonPostCheck + if runtime != "" && !r.waitForService(runtime) { + post.reasons = append(post.reasons, upgradeReasonPostCheck) } + r.verifyInstallTarget(&post) + // A configuration file the snapshot read and that is now gone is a change the run can state + // exactly. A file that still exists but cannot be read is not a change: the run cannot tell + // whether it differs, so that is a post-check finding of its own rather than a claimed change. digest, err := upgradeFileDigest(upgradeConfigPath(r.e)) - if err != nil || digest != r.beforeConfig { + switch { + case errors.Is(err, os.ErrNotExist): + r.note(upgradeStepPostCheck, nil, 1, "", fmt.Errorf("the configuration file is gone")) + post.reasons = append(post.reasons, upgradeReasonConfigChanged) + post.configChanged = true + case err != nil: + r.note(upgradeStepPostCheck, nil, 1, "", fmt.Errorf("the configuration file could not be read: %w", err)) + post.reasons = append(post.reasons, upgradeReasonPostCheck) + case digest != r.beforeConfig: r.note(upgradeStepPostCheck, nil, 1, "", fmt.Errorf("the configuration file changed")) - return upgradeExitPostCheck, upgradeReasonPostCheck + post.reasons = append(post.reasons, upgradeReasonConfigChanged) + post.configChanged = true + } + if len(post.reasons) == 0 { + return post + } + post.code = upgradeExitPostCheck + post.reason = post.reasons[0] + return post +} + +// verifyInstallTarget is what the post-check compares the pointer and the running service with: the +// runtime the update installed, or - when it installed none - the runtime it replaced. It records +// every mismatch it finds; a mismatch reason is kept once however many times this runs. +func (r *upgradeRunState) verifyInstallTarget(post *upgradePostCheck) { + pointer, err := upgradePointerTarget(r.e) + switch { + case err != nil: + r.note(upgradeStepPostCheck, nil, 1, "", err) + r.mismatch(post) + case r.promoted && r.installed == "": + r.note(upgradeStepPostCheck, nil, 1, "", fmt.Errorf("the update named no runtime it installed, so nothing shows the pointer names what it installed")) + r.mismatch(post) + case r.promoted && !upgradeSameDirectory(pointer, r.installed): + r.note(upgradeStepPostCheck, nil, 1, "", fmt.Errorf("the runtime pointer names %s, not %s, the runtime the update installed", pointer, r.installed)) + r.mismatch(post) + case r.promoted && r.serviceUp && !upgradeSameDirectory(r.startFrom, r.installed): + // The pointer names the runtime the update installed, but the service did not come back on it: + // the restart fell back to the runtime the pointer named before the stop. A pointer that names + // one runtime while the service runs another is the disagreement this step exists to catch, so + // the recovery is recorded as a mismatch rather than a success. + r.note(upgradeStepPostCheck, nil, 1, "", fmt.Errorf("the service came back on %s, not %s, the runtime the update installed", r.startFrom, r.installed)) + r.mismatch(post) + case !r.promoted && r.previous != "" && !upgradeSameDirectory(pointer, r.previous): + // The update put no runtime in service, so the pointer has to still name the runtime it + // replaced: that is the rollback, and its version is the previous one by design. A pointer + // that names anything else was changed by something other than this run's update, and nothing + // here can show what is in service, so the run refuses rather than calling it a rollback. + r.note(upgradeStepPostCheck, nil, 1, "", fmt.Errorf("the update did not put a runtime in service, and the pointer names %s, not %s, the runtime it replaced", pointer, r.previous)) + r.mismatch(post) + default: + // The version is compared only against a runtime the update put in service. An update that + // did not land leaves the pointer on the runtime it replaced, whose version is the previous + // one by design: that is the rollback, not a mismatch. + if r.promoted { + version, code, err := r.command(r.ctx, upgradeCommandTimeout, upgradeStepPostCheck, filepath.Join(pointer, "bin", "crw"), "--version") + got := strings.TrimSpace(version) + if err != nil || code != 0 || got != r.version { + r.note(upgradeStepPostCheck, nil, 1, version, fmt.Errorf("the runtime's crw reports the version %q, not %q, the version the archive carried", got, r.version)) + r.mismatch(post) + } + } + } +} + +// mismatch records a pointer-and-runtime disagreement once, however many times the check runs. +func (r *upgradeRunState) mismatch(post *upgradePostCheck) { + post.mismatch = true + if !slices.Contains(post.reasons, upgradeReasonRuntimeMismatch) { + post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) + } +} + +// waitForService reads the service status again every upgradeServiceInterval until it reports the +// daemon running and matching, or upgradeServiceBudget passes; the last answer is recorded either +// way. The wait is measured on the wall clock, not on Env.Now: a test's fixed clock must not be +// able to stop it ending. +func (r *upgradeRunState) waitForService(runtime string) bool { + ctx, cancel := context.WithTimeout(r.ctx, upgradeServiceBudget) + defer cancel() + relay := filepath.Join(runtime, "bin", "codex-session-relay") + last := "" + for { + out, code, err := r.command(ctx, upgradeCommandTimeout, upgradeStepPostCheck, relay, + "--state", r.state, "--socket", r.cfg.Relay.Socket, "service", "status") + last = out + if err == nil && code == 0 && upgradeServiceRunning(out) { + return true + } + if ctx.Err() != nil { + r.note(upgradeStepPostCheck, nil, 1, last, fmt.Errorf("the service did not report itself running and matching within %s", upgradeServiceBudget)) + return false + } + if !upgradeWait(ctx, upgradeServiceInterval) { + r.note(upgradeStepPostCheck, nil, 1, last, fmt.Errorf("the wait for the service ended before it reported itself running and matching")) + return false + } + } +} + +// upgradeWait waits for one interval, or until ctx ends; false when the context ended first. +func upgradeWait(ctx context.Context, d time.Duration) bool { + timer := time.NewTimer(d) + defer timer.Stop() + select { + case <-timer.C: + return true + case <-ctx.Done(): + return false } - return 0, "" } // upgradeServiceRunning reports whether a status answer says the daemon runs and matches. @@ -370,6 +682,20 @@ func upgradeServiceRunning(out string) bool { return answer.LaunchPolicy.MatchesRunning == "same" } +// upgradeSameDirectory reports whether two spellings name the same directory, resolving both so a +// path through a symbolic link compares equal to its target. +func upgradeSameDirectory(a, b string) bool { + if a == b { + return true + } + resolvedA, errA := filepath.EvalSymlinks(a) + resolvedB, errB := filepath.EvalSymlinks(b) + if errA != nil || errB != nil { + return false + } + return resolvedA == resolvedB +} + // upgradePointerTarget is where the runtime pointer resolves, as a real path. func upgradePointerTarget(e *Env) (string, error) { path := filepath.Join(coreHomeDir(e, "HOME", ""), upgradeRuntimeDir, "current") diff --git a/internal/manage/upgrade_steps_test.go b/internal/manage/upgrade_steps_test.go index bd6198174b..bf4dc050d6 100644 --- a/internal/manage/upgrade_steps_test.go +++ b/internal/manage/upgrade_steps_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "strings" "testing" + "time" ) // C1: a stop at each of steps 1 to 4 calls no later step, proven by the fakes' received calls. @@ -14,15 +15,14 @@ import ( func TestUpgradeStopsBeforeTheNextStep(t *testing.T) { commit := upgradeGoodCommit badSums := func(t *testing.T, h *upgradeEnv) { - if err := os.WriteFile(filepath.Join(h.release, upgradeSumsName), []byte("deadbeef crw_0.4.0_linux_amd64.tar.gz\n"), 0o600); err != nil { + if err := os.WriteFile(filepath.Join(h.release, upgradeSumsName), []byte("deadbeef "+upgradeArchiveName+"\n"), 0o600); err != nil { t.Fatal(err) } } gate := func(body string) map[string]upgradeGhAnswer { - return map[string]upgradeGhAnswer{ - "repos/owner/repo/commits/eb2567df7": {Body: "{\"sha\":\"" + commit + "\"}"}, - "repos/owner/repo/commits/" + commit + "/check-runs": {Body: body}, - } + out := upgradeGhPaths(commit) + out["repos/owner/repo/commits/"+commit+"/check-runs"] = upgradeGhAnswer{Body: body} + return out } for _, tc := range []struct { name string @@ -32,13 +32,13 @@ func TestUpgradeStopsBeforeTheNextStep(t *testing.T) { reason string absent []string }{ - {"step 1", upgradeHarnessOptions{version: "v0.4.0-4633-geb2567df7", gh: upgradeGhPaths(commit), pointer: true}, badSums, + {"step 1", upgradeHarnessOptions{gh: upgradeGhPaths(commit), pointer: true}, badSums, 2, upgradeReasonSumsFailed, []string{"commits/", "check-runs", "install", "service"}}, - {"step 2", upgradeHarnessOptions{version: "v0.4.0-4633-geb2567df7", gh: map[string]upgradeGhAnswer{}, pointer: true}, nil, + {"step 2", upgradeHarnessOptions{gh: map[string]upgradeGhAnswer{}, pointer: true}, nil, 2, upgradeReasonCommitUnknown, []string{"check-runs", "install", "service"}}, - {"step 3", upgradeHarnessOptions{version: "v0.4.0-4633-geb2567df7", gh: gate("{\"check_runs\":[]}"), pointer: true}, nil, + {"step 3", upgradeHarnessOptions{gh: gate("{\"check_runs\":[]}"), pointer: true}, nil, 2, upgradeReasonDevGate, []string{"install", "service"}}, - {"step 4", upgradeHarnessOptions{version: "v0.4.0-4633-geb2567df7", openAttempts: 1, gh: upgradeGhPaths(commit), pointer: true}, nil, + {"step 4", upgradeHarnessOptions{openAttempts: 1, gh: upgradeGhPaths(commit), pointer: true}, nil, 3, upgradeReasonOpenAttempts, []string{"install", "service"}}, } { t.Run(tc.name, func(t *testing.T) { @@ -47,7 +47,7 @@ func TestUpgradeStopsBeforeTheNextStep(t *testing.T) { tc.break_(t, h) } if code := h.run("--release-dir", h.release); code != tc.code { - t.Fatalf("exit %d, want %d", code, tc.code) + t.Fatalf("exit %d, want %d; the record is %+v", code, tc.code, h.recordOf(t)) } if got := h.recordOf(t).Reason; got != tc.reason { t.Errorf("reason %q, want %q", got, tc.reason) @@ -62,10 +62,13 @@ func TestUpgradeStopsBeforeTheNextStep(t *testing.T) { } } -// C4 and C5: a dry run performs steps 1 to 4, calls no service command, and leaves its record -// and extract directory behind; a second run in the same second refuses rather than overwriting. +// C4, C5 and C8: a dry run performs steps 1 to 4, calls no service command, and leaves its record +// and extract directory behind. Two runs in one second cannot share a run directory, and the +// injected clock is what decides that: the same second refuses the second run, and one second +// later the run takes a directory of its own. Pinning Env.Now is what makes this deterministic: +// the wall clock crossing a second boundary between the two runs changes nothing. func TestUpgradeDryRunCallsNoServiceCommand(t *testing.T) { - h := upgradeHarness(t, upgradeHarnessOptions{version: "v0.4.0-4633-geb2567df7", gh: upgradeGhPaths(upgradeGoodCommit)}) + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), pointer: true}) if code := h.run("--release-dir", h.release, "--dry-run"); code != 0 { t.Fatalf("exit %d", code) } @@ -81,20 +84,34 @@ func TestUpgradeDryRunCallsNoServiceCommand(t *testing.T) { if _, err := os.Stat(filepath.Join(record.ExtractDir, "crw")); err != nil { t.Errorf("the extracted crw is gone: %v", err) } - if code := h.run("--release-dir", h.release, "--dry-run"); code == 0 { - t.Fatal("a second run in the same second overwrote the first") - } + t.Run("the same second refuses", func(t *testing.T) { + if code := h.run("--release-dir", h.release, "--dry-run"); code == 0 { + t.Fatal("a second run in the same second overwrote the first") + } + if got := h.recordDirs(); len(got) != 1 { + t.Errorf("the run directories are %v, want the one run", got) + } + }) + t.Run("the next second takes its own directory", func(t *testing.T) { + h.advance(time.Second) + if code := h.run("--release-dir", h.release, "--dry-run"); code != 0 { + t.Fatalf("exit %d, want 0", code) + } + if got := h.recordDirs(); len(got) != 2 { + t.Errorf("the run directories are %v, want two", got) + } + }) } -// C2: start is called even when the update fails. +// C2: start is called even when the update fails, and the service comes back on the runtime the +// pointer still names. func TestUpgradeStartsAfterAFailedUpdate(t *testing.T) { - h := upgradeHarness(t, upgradeHarnessOptions{version: "v0.4.0-4633-geb2567df7", gh: upgradeGhPaths(upgradeGoodCommit), pointer: true, installExit: 1}) + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), pointer: true, installExit: 1}) if code := h.run("--release-dir", h.release); code == 0 { t.Fatal("a failed update reported success") } - joined := strings.Join(h.crwCalls(), " ") - if !strings.Contains(joined, "service stop") || !strings.Contains(joined, "service start") { - t.Errorf("a failed update did not stop then start: %q", joined) + if !h.calledFrom(filepath.Join(h.previous, "bin", "codex-session-relay"), "service start") { + t.Errorf("a failed update did not stop then start: %q", h.callLines()) } } @@ -102,19 +119,20 @@ func TestUpgradeStartsAfterAFailedUpdate(t *testing.T) { // differential is TestUpgradeSucceedsOnAHealthyHost: the same run exits 0 with the configuration // left alone, so this exit 4 is the configuration comparison, not the status check. func TestUpgradeExitsFourWhenTheConfigChanged(t *testing.T) { - h := upgradeHarness(t, upgradeHarnessOptions{version: "v0.4.0-4633-geb2567df7", gh: upgradeGhPaths(upgradeGoodCommit), pointer: true, mutateConfig: true}) + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), pointer: true, + produceRuntime: true, pointAtIt: true, mutateConfig: true}) if code := h.run("--release-dir", h.release); code != upgradeExitPostCheck { - t.Fatalf("exit %d, want %d", code, upgradeExitPostCheck) + t.Fatalf("exit %d, want %d; the record is %+v", code, upgradeExitPostCheck, h.recordOf(t)) } - if got := h.recordOf(t).Reason; got != upgradeReasonPostCheck { - t.Errorf("reason %q, want %q", got, upgradeReasonPostCheck) + if got := h.recordOf(t).Reason; got != upgradeReasonConfigChanged { + t.Errorf("reason %q, want %q", got, upgradeReasonConfigChanged) } } // A stop that runs but fails stops the run before the runtime is replaced: the record names the // stop and no install follows. func TestUpgradeRefusesAFailedStop(t *testing.T) { - h := upgradeHarness(t, upgradeHarnessOptions{version: "v0.4.0-4633-geb2567df7", gh: upgradeGhPaths(upgradeGoodCommit), pointer: true, stopExit: 1}) + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit), pointer: true, stopExit: 1}) if code := h.run("--release-dir", h.release); code != upgradeExitRefused { t.Fatalf("exit %d, want %d", code, upgradeExitRefused) } @@ -130,7 +148,7 @@ func TestUpgradeRefusesAFailedStop(t *testing.T) { // the service is touched. func TestUpgradeRefusesBeforeTheSteps(t *testing.T) { t.Run("repository", func(t *testing.T) { - h := upgradeHarness(t, upgradeHarnessOptions{version: "v0.4.0-4633-geb2567df7", gh: upgradeGhPaths(upgradeGoodCommit)}) + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit)}) old := upgradeConfig upgradeConfig = func(e *Env) *Config { return coreDefaults(e) } defer func() { upgradeConfig = old }() @@ -144,102 +162,14 @@ func TestUpgradeRefusesBeforeTheSteps(t *testing.T) { } }) t.Run("pointer", func(t *testing.T) { - h := upgradeHarness(t, upgradeHarnessOptions{version: "v0.4.0-4633-geb2567df7", gh: upgradeGhPaths(upgradeGoodCommit)}) + h := upgradeHarness(t, upgradeHarnessOptions{gh: upgradeGhPaths(upgradeGoodCommit)}) if code := h.run("--release-dir", h.release); code != upgradeExitRefused { t.Fatalf("exit %d, want %d", code, upgradeExitRefused) } - for _, line := range h.crwCalls() { + for _, line := range h.callArgs() { if strings.Contains(line, "service") { t.Errorf("a missing pointer still touched the service: %q", line) } } }) } - -// The command prints its usage: exit 0 for the help flags, exit 2 for anything unusable. -func TestUpgradeCommandPrintsItsUsage(t *testing.T) { - for _, tc := range []struct { - args []string - code int - }{ - {[]string{"-h"}, 0}, {[]string{"--help"}, 0}, - {[]string{"nope"}, usageExit}, {nil, usageExit}, - } { - var out, errOut strings.Builder - if code := Run(context.Background(), append([]string{"runtime-upgrade"}, tc.args...), strings.NewReader(""), &out, &errOut); code != tc.code { - t.Errorf("%q: exit %d, want %d", tc.args, code, tc.code) - } - } -} - -// A released tag version resolves through the tag ref (and its v-prefixed form); a git-describe -// version resolves through its short hash. -func TestUpgradeCommitRefs(t *testing.T) { - for _, tc := range []struct { - version string - want string - }{ - {"v0.4.0-4633-geb2567df7", "eb2567df7"}, - {"v0.4.1", "v0.4.1"}, - {"0.4.1", "0.4.1,v0.4.1"}, - {"", ""}, - } { - if got := strings.Join(upgradeCommitRefs(tc.version), ","); got != tc.want { - t.Errorf("%q: %q, want %q", tc.version, got, tc.want) - } - } -} - -// An entry through an existing symlink is refused, so a crafted archive cannot reach a file -// outside the extract directory. -func TestUpgradeExtractRefusesASymlinkEscape(t *testing.T) { - dir, outside := t.TempDir(), filepath.Join(t.TempDir(), "outside") - if err := os.WriteFile(outside, []byte("original"), 0o600); err != nil { - t.Fatal(err) - } - if err := os.Symlink(outside, filepath.Join(dir, "crw")); err != nil { - t.Fatal(err) - } - archive := filepath.Join(t.TempDir(), "a.tar.gz") - if err := os.WriteFile(archive, upgradeTarGz(t, map[string]string{"crw": "overwritten"}), 0o600); err != nil { - t.Fatal(err) - } - if err := upgradeExtract(archive, dir); err == nil { - t.Fatal("an entry through a symlink was accepted") - } - if data, err := os.ReadFile(outside); err != nil || string(data) != "original" { - t.Errorf("the file outside the extract directory changed: %q %v", data, err) - } -} - -// The full success flow for both version shapes: with the pointer in place, an update that -// succeeds and a service that runs and matches, every step runs and the command exits 0. -func TestUpgradeSucceedsOnAHealthyHost(t *testing.T) { - tag := upgradeGhPaths(upgradeGoodCommit) - delete(tag, "repos/owner/repo/commits/eb2567df7") - tag["repos/owner/repo/commits/v0.4.1"] = upgradeGhAnswer{Body: "{\"sha\":\"" + upgradeGoodCommit + "\"}"} - for _, tc := range []struct { - version string - gh map[string]upgradeGhAnswer - }{{"v0.4.0-4633-geb2567df7", upgradeGhPaths(upgradeGoodCommit)}, {"v0.4.1", tag}} { - h := upgradeHarness(t, upgradeHarnessOptions{version: tc.version, gh: tc.gh, pointer: true}) - if code := h.run("--release-dir", h.release); code != 0 { - t.Fatalf("%s: exit %d, want 0", tc.version, code) - } - record := h.recordOf(t) - if record.Outcome != "ok" || record.Reason != "" { - t.Errorf("%s: the record is %+v", tc.version, record) - } - var steps []string - for _, s := range record.Steps { - steps = append(steps, s.Step) - } - joined := strings.Join(steps, ",") - for _, want := range []string{upgradeStepSums, upgradeStepExtract, upgradeStepCommit, upgradeStepDevGate, - upgradeStepAttempts, upgradeStepSnapshot, upgradeStepStop, upgradeStepUpdate, upgradeStepStart, upgradeStepPostCheck} { - if !strings.Contains(joined, want) { - t.Errorf("%s: the record does not name the step %q: %v", tc.version, want, joined) - } - } - } -} diff --git a/internal/manage/upgrade_test.go b/internal/manage/upgrade_test.go index 812a0cb159..6203bf0a6a 100644 --- a/internal/manage/upgrade_test.go +++ b/internal/manage/upgrade_test.go @@ -12,33 +12,133 @@ import ( "fmt" "os" "path/filepath" + "strconv" "strings" "syscall" "testing" + "time" _ "modernc.org/sqlite" ) -// upgradeEnv is one run's environment: temporary homes, a fake crw and gh, a release directory -// holding one verified archive, and a temporary relay store. +// The upgrade harness: temporary homes, one fake crw/relay script placed everywhere a runtime is +// reached, a release directory holding one verified archive, a temporary relay store, and a clock +// the test holds. Nothing here reads or writes a real home, runtime, service or relay store. + +const ( + // upgradeGoodCommit is the commit the fake gh resolves a released version to. + upgradeGoodCommit = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + // upgradeArchiveName is the release asset the harness writes. + upgradeArchiveName = "crw_0.4.0_linux_amd64.tar.gz" + // upgradeArchiveVersion is the version that asset's name carries, and so the version the + // installer's runtime directory rule would use. + upgradeArchiveVersion = "0.4.0" + // upgradeStatusSame and upgradeStatusUnknown are the service status answers the fake gives. + upgradeStatusSame = `{"running":true,"launchPolicy":{"matchesRunning":"same"}}` + upgradeStatusUnknown = `{"running":true,"launchPolicy":{"matchesRunning":"unknown"}}` + // upgradeFakeVersion is the version the fakes answer with unless a test says otherwise. + upgradeFakeVersion = "v0.4.0-4633-geb2567df7" + // upgradeClockStart is where the injected clock starts, so two runs of one test share a second + // whatever the wall clock does. + upgradeClockStart = "2026-01-01T00:00:00Z" +) + +// upgradeEnv is one run's environment. type upgradeEnv struct { t *testing.T home string codex string state string release string - calls string - ghCalls string + + // calls is the arguments of every call the fake answered; exeCalls is one line per call as + // "", so a test can see which runtime a call came from. + calls string + exeCalls string + ghCalls string + + statusAnswers string + statusCounter string + // started marks that a service start succeeded, so a status read reports a service that is + // really up rather than one the script was merely asked about. + started string + pointerLink string + + // previous is the runtime directory the owned pointer names before the run, and installed the + // runtime directory the fake update reports it produced. + previous string + installed string + // other is a runtime that is neither the one the pointer names before the run nor the one the + // update produces, used to show that a pointer moved to a third runtime is refused. + other string + + // script is the fake, and now is the clock every run reads. + script string + now time.Time } type upgradeHarnessOptions struct { - version string + // version is what the release archive's crw answers --version with. + version string + // installedVersion is what the runtime the update produces answers --version with; empty + // means the archive's version. + installedVersion string + // previousVersion is what the runtime the pointer already names answers --version with; empty + // means the archive's version. A real upgrade replaces one version with another, so the + // runtime left behind answers differently from the archive. + previousVersion string + // openAttempts is the doctor answer's contents.openAttempts. openAttempts int - gh map[string]upgradeGhAnswer - pointer bool - installExit int + // doctorUnavailable makes the doctor answer report contents it could not read. + doctorUnavailable bool + // doctorNoCount makes the doctor answer report readable contents that carry no open attempt + // count, which is the other half of the fail-closed guard. + doctorNoCount bool + // doctorServesAnotherStore makes the doctor answer name a store the relay service serves that is + // not the directory discovery selected, so the contents it read are not the served store's. + doctorServesAnotherStore bool + // servedStoreOpenAttempts is the open attempt count the served store reports when the doctor names + // another store than the selected directory. + servedStoreOpenAttempts int + gh map[string]upgradeGhAnswer + // pointer is whether the owned pointer exists before the run. + pointer bool + // installExit is the status the fake update ends with. + installExit int + // produceRuntime makes the fake update create a runtime directory and name it in its answer. + produceRuntime bool + // pointAtIt makes the fake update move the owned pointer to that directory. + pointAtIt bool + // pointAtOther makes the fake update move the owned pointer to a runtime that is neither the one + // the pointer named before the run nor the one the update produces, as a concurrent install or + // rollback would. + pointAtOther bool + // breakPointer makes the fake update remove the owned pointer, as an update that died between + // committing the new selection and placing the link would. + breakPointer bool + // mutateConfig makes the fake update append to the configuration file. mutateConfig bool - stopExit int + // unreadableConfigAfter makes the fake update leave the configuration file unreadable without + // changing its contents, so the post-check cannot tell whether it changed. + unreadableConfigAfter bool + // deleteConfigAfter makes the fake update remove the configuration file the snapshot read, which + // is a change the post-check can state exactly. + deleteConfigAfter bool + // breakInstalledStart makes the runtime the update produces fail to start the service, so the + // restart has to fall back. + breakInstalledStart bool + // breakPointerDuringWait removes the owned pointer on the second service status read, as another + // process on the host moving the pointer while the run waits for the service would. + breakPointerDuringWait bool + // installedStartAlreadyRunning makes the runtime the update produces answer its start with the + // service already up, as it does when something started that runtime between the stop and the + // restart. + installedStartAlreadyRunning bool + // stopExit is the status the fake service stop ends with. + stopExit int + // statusAnswers is the matchesRunning answer for each service status read, the last one + // repeating; empty means one answer that runs and matches. + statusAnswers []string } type upgradeGhAnswer struct { @@ -48,6 +148,9 @@ type upgradeGhAnswer struct { func upgradeHarness(t *testing.T, opts upgradeHarnessOptions) *upgradeEnv { t.Helper() + if opts.version == "" { + opts.version = upgradeFakeVersion + } home := t.TempDir() codex := filepath.Join(home, ".codex") state := filepath.Join(home, "relay-state") @@ -62,40 +165,157 @@ func upgradeHarness(t *testing.T, opts upgradeHarnessOptions) *upgradeEnv { if err := os.WriteFile(filepath.Join(codex, "config.toml"), []byte("model = x\n"), 0o600); err != nil { t.Fatal(err) } + root := filepath.Join(home, ".local", "share", "crw-runtime") + started, err := time.Parse(time.RFC3339, upgradeClockStart) + if err != nil { + t.Fatal(err) + } h := &upgradeEnv{t: t, home: home, codex: codex, state: state, release: t.TempDir(), - calls: filepath.Join(home, "crw-calls.jsonl"), ghCalls: filepath.Join(home, "gh-calls.jsonl")} - h.writeArchive(opts) - h.writeStore(opts.openAttempts) + calls: filepath.Join(home, "crw-calls.txt"), exeCalls: filepath.Join(home, "crw-exe-calls.txt"), + ghCalls: filepath.Join(home, "gh-calls.jsonl"), + statusAnswers: filepath.Join(home, "service-status-answers"), + statusCounter: filepath.Join(home, "service-status-count"), + started: filepath.Join(home, "service-started"), + pointerLink: filepath.Join(root, "current"), + previous: filepath.Join(root, "bin-"+upgradeArchiveVersion+"-000000000000"), + installed: filepath.Join(root, "bin-"+upgradeArchiveVersion+"-111111111111"), + other: filepath.Join(root, "bin-"+upgradeArchiveVersion+"-222222222222"), + now: started} + h.script = h.fakeScript(opts) h.writeFakes(opts) + h.writeStore() + h.writeArchive(opts) + previous := opts.previousVersion + if previous == "" { + previous = opts.version + } + h.installRuntime(h.previous, previous) + h.installRuntime(h.other, previous) h.installPointer(opts.pointer) return h } -func (h *upgradeEnv) writeArchive(opts upgradeHarnessOptions) { - h.t.Helper() - body := upgradeTarGz(h.t, map[string]string{"crw": fakeCRWScript(opts.version, opts.installExit, opts.mutateConfig)}) - name := "crw_0.4.0_linux_amd64.tar.gz" - if err := os.WriteFile(filepath.Join(h.release, name), body, 0o600); err != nil { - h.t.Fatal(err) +// advance moves the injected clock, so a test can put two runs in different seconds. +func (h *upgradeEnv) advance(d time.Duration) { h.now = h.now.Add(d) } + +// pointerTarget is where the owned pointer resolves, or an error when it resolves nowhere. +func (h *upgradeEnv) pointerTarget() (string, error) { + target, err := filepath.EvalSymlinks(h.pointerLink) + if err != nil { + return "", err } - sum := sha256.Sum256(body) - sums := hex.EncodeToString(sum[:]) + " " + name + "\n" - if err := os.WriteFile(filepath.Join(h.release, upgradeSumsName), []byte(sums), 0o600); err != nil { + return target, nil +} + +// fakeScript is the one fake the harness puts everywhere a crw or a codex-session-relay is +// reached: on PATH, in the runtime the pointer names, in the runtime the update produces, and +// inside the release archive. It records its call, answers the version of the runtime it sits in, +// the relay doctor and the service status, and its install arm does what the options say an update +// does. +func (h *upgradeEnv) fakeScript(opts upgradeHarnessOptions) string { + installed := opts.installedVersion + if installed == "" { + installed = opts.version + } + doctor := "{\"stateSelection\":{\"path\":" + jsonString(h.state) + "},\"contents\":{\"available\":true,\"openAttempts\":" + strconv.Itoa(opts.openAttempts) + "}}" + if opts.doctorUnavailable { + doctor = "{\"stateSelection\":{\"path\":" + jsonString(h.state) + "},\"contents\":{\"available\":false,\"openAttempts\":null}}" + } + if opts.doctorNoCount { + doctor = "{\"stateSelection\":{\"path\":" + jsonString(h.state) + "},\"contents\":{\"available\":true,\"openAttempts\":null}}" + } + if opts.doctorServesAnotherStore { + doctor = "{\"stateSelection\":{\"path\":" + jsonString(h.state) + "},\"contents\":{\"available\":true,\"openAttempts\":0}," + + "\"serviceStore\":{\"stateDirectory\":" + jsonString(h.state+"-served") + ",\"live\":true}}" + } + doctorServed := "{\"stateSelection\":{\"path\":" + jsonString(h.state+"-served") + "},\"contents\":{\"available\":true,\"openAttempts\":" + strconv.Itoa(opts.servedStoreOpenAttempts) + "}}" + answers := opts.statusAnswers + if len(answers) == 0 { + answers = []string{upgradeStatusSame} + } + if err := os.WriteFile(h.statusAnswers, []byte(strings.Join(answers, "\n")+"\n"), 0o600); err != nil { h.t.Fatal(err) } + bin := filepath.Join(h.installed, "bin") + var b strings.Builder + b.WriteString("#!/bin/sh\n") + b.WriteString("printf '%s\\n' \"$*\" >> " + coreShellQuote(h.calls) + "\n") + b.WriteString("printf '%s\\t%s\\n' \"$0\" \"$*\" >> " + coreShellQuote(h.exeCalls) + "\n") + // The version a runtime answers with is the file beside it, so the archive, the runtime the + // pointer already names and the runtime the update produces can each report their own. + b.WriteString("dir=" + "$" + "{0%/*}\n") + b.WriteString("case \"$*\" in\n") + b.WriteString("*\"service stop\"*) exit " + strconv.Itoa(opts.stopExit) + ";;\n") + b.WriteString("*\"service start\"*)\n") + if opts.breakInstalledStart { + // The runtime the update produced cannot start the service: its relay executable fails. + b.WriteString(" [ \"$dir\" = " + coreShellQuote(filepath.Join(h.installed, "bin")) + " ] && exit 1\n") + } + if opts.installedStartAlreadyRunning { + // The service is already up on the runtime the update produced: start refuses with + // already_running rather than launching a second one. The started marker is set so a status + // read reports a service that is really up. + b.WriteString(" [ \"$dir\" = " + coreShellQuote(filepath.Join(h.installed, "bin")) + " ] && { printf '%s\\n' \"$dir\" > " + coreShellQuote(h.started) + "; printf '%s\\n' " + coreShellQuote("{\"ok\":false,\"reason\":\"already_running\"}") + "; exit 2; }\n") + } + b.WriteString(" printf '%s\\n' \"$dir\" > " + coreShellQuote(h.started) + "\n") + b.WriteString(" exit 0;;\n") + b.WriteString("*\"service status\"*)\n") + b.WriteString(" n=$(cat " + coreShellQuote(h.statusCounter) + " 2>/dev/null || printf '0')\n") + b.WriteString(" n=$((n + 1))\n") + if opts.breakPointerDuringWait { + b.WriteString(" [ \"$n\" = \"2\" ] && rm -f " + coreShellQuote(h.pointerLink) + "\n") + } + // A status read before anything started the service reports it as down, so a run that never + // restarts cannot pass the post-check by reading an answer that was scripted for another case. + b.WriteString(" [ -f " + coreShellQuote(h.started) + " ] || { printf '%s\\n' " + coreShellQuote("{\"running\":false,\"launchPolicy\":{\"matchesRunning\":\"different\"}}") + "; exit 0; }\n") + b.WriteString(" lines=$(wc -l < " + coreShellQuote(h.statusAnswers) + ")\n") + b.WriteString(" [ \"$n\" -gt \"$lines\" ] && n=$lines\n") + b.WriteString(" printf '%s\\n' \"$n\" > " + coreShellQuote(h.statusCounter) + "\n") + b.WriteString(" sed -n \"" + "$" + "{n}p\" " + coreShellQuote(h.statusAnswers) + "\n") + b.WriteString(" ;;\n") + b.WriteString("*doctor*)\n") + b.WriteString(" case \"$*\" in *" + h.state + "-served*) printf '%s\\n' " + coreShellQuote(doctorServed) + ";; *) printf '%s\\n' " + coreShellQuote(doctor) + ";; esac ;;\n") + b.WriteString("*\"install update\"*)\n") + if opts.breakPointer { + b.WriteString(" rm -f " + coreShellQuote(h.pointerLink) + "\n") + } + if opts.pointAtOther { + b.WriteString(" ln -sfn " + coreShellQuote(h.other) + " " + coreShellQuote(h.pointerLink) + "\n") + } + if opts.mutateConfig { + b.WriteString(" printf 'changed\\n' >> \"$CODEX_HOME/config.toml\"\n") + } + if opts.unreadableConfigAfter { + b.WriteString(" chmod 000 \"$CODEX_HOME/config.toml\"\n") + } + if opts.deleteConfigAfter { + b.WriteString(" rm -f \"$CODEX_HOME/config.toml\"\n") + } + if opts.produceRuntime { + b.WriteString(" mkdir -p " + coreShellQuote(bin) + "\n") + b.WriteString(" cp \"$0\" " + coreShellQuote(filepath.Join(bin, "crw")) + "\n") + b.WriteString(" cp \"$0\" " + coreShellQuote(filepath.Join(bin, "codex-session-relay")) + "\n") + b.WriteString(" chmod 755 " + coreShellQuote(filepath.Join(bin, "crw")) + " " + coreShellQuote(filepath.Join(bin, "codex-session-relay")) + "\n") + b.WriteString(" printf '%s\\n' " + coreShellQuote(installed) + " > " + coreShellQuote(filepath.Join(bin, "version")) + "\n") + if opts.pointAtIt { + b.WriteString(" ln -sfn " + coreShellQuote(h.installed) + " " + coreShellQuote(h.pointerLink) + "\n") + } + b.WriteString(" printf '%s\\n' " + coreShellQuote("{\"environment\":"+jsonString(h.installed)+"}") + "\n") + } + b.WriteString(" exit " + strconv.Itoa(opts.installExit) + ";;\n") + b.WriteString("*\"--version\"*) cat \"$dir/version\" 2>/dev/null || printf '%s\\n' " + coreShellQuote(opts.version) + " ;;\n") + b.WriteString("esac\n") + b.WriteString("exit 0\n") + return b.String() } -// fakeCRWScript is the extracted archive's crw: it answers --version and ends install with the -// given status, optionally having changed the configuration first. -func fakeCRWScript(version string, installExit int, mutateConfig bool) string { - mutate := "" - if mutateConfig { - mutate = "printf 'changed\\n' >> \"$CODEX_HOME/config.toml\"\n" - } - return "#!/bin/sh\n" + - "if [ \"$1\" = \"--version\" ]; then printf '%s\\n' '" + version + "'; exit 0; fi\n" + - "if [ \"$1\" = \"install\" ]; then " + mutate + "exit " + fmt.Sprint(installExit) + "; fi\n" + - "exit 0\n" +// jsonString is s as a JSON string, for a fake answer built by concatenation. +func jsonString(s string) string { + data, err := json.Marshal(s) + if err != nil { + panic(err) + } + return string(data) } func upgradeTarGz(t *testing.T, files map[string]string) []byte { @@ -120,7 +340,25 @@ func upgradeTarGz(t *testing.T, files map[string]string) []byte { return buf.Bytes() } -func (h *upgradeEnv) writeStore(openAttempts int) { +// writeArchive writes the release archive and its SHA256SUMS. The archive holds the fake crw and +// the version file beside it, so the extracted crw reports the archive's version. +func (h *upgradeEnv) writeArchive(opts upgradeHarnessOptions) { + h.t.Helper() + body := upgradeTarGz(h.t, map[string]string{"crw": h.script, "version": opts.version + "\n"}) + if err := os.WriteFile(filepath.Join(h.release, upgradeArchiveName), body, 0o600); err != nil { + h.t.Fatal(err) + } + sum := sha256.Sum256(body) + sums := hex.EncodeToString(sum[:]) + " " + upgradeArchiveName + "\n" + if err := os.WriteFile(filepath.Join(h.release, upgradeSumsName), []byte(sums), 0o600); err != nil { + h.t.Fatal(err) + } +} + +// writeStore writes a relay store whose attempts are all settled. The run reads open attempts from +// the relay doctor, so a run that went back to reading this store would see none: the doctor cases +// are what pin the count. +func (h *upgradeEnv) writeStore() { h.t.Helper() db, err := sql.Open("sqlite", filepath.Join(h.state, "relay.sqlite3")) if err != nil { @@ -130,50 +368,34 @@ func (h *upgradeEnv) writeStore(openAttempts int) { if _, err := db.Exec("CREATE TABLE attempts (request_id TEXT, internal_state TEXT)"); err != nil { h.t.Fatal(err) } - for i := 0; i < 3+openAttempts; i++ { - state := "settled" - if i >= 3 { - state = "in_flight" - } - if _, err := db.Exec("INSERT INTO attempts VALUES (?, ?)", fmt.Sprintf("row-%d", i), state); err != nil { + for i := 0; i < 3; i++ { + if _, err := db.Exec("INSERT INTO attempts VALUES (?, ?)", fmt.Sprintf("row-%d", i), "settled"); err != nil { h.t.Fatal(err) } } } -// writeFakes places the fake crw and gh on PATH: crw records its arguments and answers doctor, -// --version and service status; gh answers the API paths asked. +// writeFakes places the fake crw and gh on PATH. func (h *upgradeEnv) writeFakes(opts upgradeHarnessOptions) { h.t.Helper() bin := filepath.Join(h.home, "bin") if err := os.MkdirAll(bin, 0o700); err != nil { h.t.Fatal(err) } - // Branches match the whole argument list: the relay is called as - // "codex-session-relay --state S --socket K service status", flags first. - stop := "" - if opts.stopExit != 0 { - stop = "*\"service stop\"*) exit " + fmt.Sprint(opts.stopExit) + ";;\n" - } - crw := "#!/bin/sh\n" + - "printf '%s\\n' \"$*\" >> " + coreShellQuote(h.calls) + "\n" + - "case \"$*\" in\n" + stop + - "*doctor*) printf '%s\\n' " + coreShellQuote("{\"stateSelection\":{\"path\":\""+h.state+"\"}}") + "; exit 0;;\n" + - "*\"--version\"*) printf '%s\\n' " + coreShellQuote("v0.4.0-4633-geb2567df7") + "; exit 0;;\n" + - "*\"service status\"*) printf '%s\\n' " + coreShellQuote("{\"running\":true,\"launchPolicy\":{\"matchesRunning\":\"same\"}}") + "; exit 0;;\n" + - "esac\n" + - "exit 0\n" syscall.ForkLock.RLock() - writeErr := os.WriteFile(filepath.Join(bin, "crw"), []byte(crw), 0o700) + writeErr := os.WriteFile(filepath.Join(bin, "crw"), []byte(h.script), 0o700) syscall.ForkLock.RUnlock() if writeErr != nil { h.t.Fatal(writeErr) } + if err := os.WriteFile(filepath.Join(bin, "version"), []byte(opts.version+"\n"), 0o600); err != nil { + h.t.Fatal(err) + } var body strings.Builder body.WriteString("#!/bin/sh\nprintf '%s\\n' \"$*\" >> " + coreShellQuote(h.ghCalls) + "\n") body.WriteString("case \"$2\" in\n") for path, answer := range opts.gh { - body.WriteString(coreShellQuote(path) + ") printf '%s\\n' " + coreShellQuote(answer.Body) + "; exit " + fmt.Sprint(answer.Exit) + ";;\n") + body.WriteString(coreShellQuote(path) + ") printf '%s\\n' " + coreShellQuote(answer.Body) + "; exit " + strconv.Itoa(answer.Exit) + ";;\n") } body.WriteString("*) exit 1;;\nesac\n") syscall.ForkLock.RLock() @@ -185,34 +407,41 @@ func (h *upgradeEnv) writeFakes(opts upgradeHarnessOptions) { h.t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH")) } -// installPointer places the runtime pointer, with the fakes in its bin so the snapshot and stop -// steps call them too. -func (h *upgradeEnv) installPointer(want bool) { +// installRuntime places a runtime directory holding the fake crw, the fake relay and the version +// the runtime reports. +func (h *upgradeEnv) installRuntime(dir, version string) { h.t.Helper() - if !want { - return - } - target := filepath.Join(h.home, ".local", "share", "crw-runtime", "bin-0.4.0-aaaaaaaaaaaa") - if err := os.MkdirAll(filepath.Join(target, "bin"), 0o700); err != nil { - h.t.Fatal(err) - } - body, err := os.ReadFile(filepath.Join(h.home, "bin", "crw")) - if err != nil { + bin := filepath.Join(dir, "bin") + if err := os.MkdirAll(bin, 0o700); err != nil { h.t.Fatal(err) } for _, name := range []string{"crw", "codex-session-relay"} { syscall.ForkLock.RLock() - writeErr := os.WriteFile(filepath.Join(target, "bin", name), body, 0o700) + writeErr := os.WriteFile(filepath.Join(bin, name), []byte(h.script), 0o700) syscall.ForkLock.RUnlock() if writeErr != nil { h.t.Fatal(writeErr) } } - if err := os.Symlink(target, filepath.Join(h.home, ".local", "share", "crw-runtime", "current")); err != nil { + if err := os.WriteFile(filepath.Join(bin, "version"), []byte(version+"\n"), 0o600); err != nil { + h.t.Fatal(err) + } +} + +// installPointer places the owned pointer at the runtime the harness calls previous. +func (h *upgradeEnv) installPointer(want bool) { + h.t.Helper() + if !want { + return + } + if err := os.Symlink(h.previous, h.pointerLink); err != nil { h.t.Fatal(err) } } +// run drives the registered command with this harness's own Env, whose clock the harness holds: +// the run directory is named for the second the run started in, so a test that needs two runs +// inside one second pins Env.Now instead of racing the wall clock. func (h *upgradeEnv) run(args ...string) int { h.t.Helper() old := upgradeConfig @@ -226,20 +455,109 @@ func (h *upgradeEnv) run(args ...string) int { } h.t.Cleanup(func() { upgradeConfig = old }) var out, errOut strings.Builder - return Run(context.Background(), append([]string{"runtime-upgrade"}, args...), strings.NewReader(""), &out, &errOut) + e := &Env{ + Stdin: strings.NewReader(""), + Stdout: &out, + Stderr: &errOut, + Getenv: os.Getenv, + Now: func() time.Time { return h.now }, + Executable: coreExecutable(), + } + defer coreForgetConfig(e) + return upgradeCommand.Run(context.Background(), e, args) +} + +// callArgs is the arguments of every call the fake answered, one string per call. +func (h *upgradeEnv) callArgs() []string { + h.t.Helper() + return upgradeLines(h.t, h.calls) +} + +// callLines is one line per call the fake answered, as "". +func (h *upgradeEnv) callLines() []string { + h.t.Helper() + return upgradeLines(h.t, h.exeCalls) } -func upgradeCallLines(t *testing.T, path string) []string { +func upgradeLines(t *testing.T, path string) []string { t.Helper() data, err := os.ReadFile(path) if err != nil && !os.IsNotExist(err) { t.Fatal(err) } - return strings.Fields(string(data)) + var out []string + for _, line := range strings.Split(strings.TrimSpace(string(data)), "\n") { + if line != "" { + out = append(out, line) + } + } + return out +} + +func (h *upgradeEnv) crwCalls() []string { return strings.Fields(strings.Join(h.callArgs(), " ")) } +func (h *upgradeEnv) ghCallLines() []string { return upgradeLines(h.t, h.ghCalls) } + +// calledFrom reports whether the fake was reached as exe and asked for the verb. +func (h *upgradeEnv) calledFrom(exe, verb string) bool { + h.t.Helper() + resolved := exe + if target, err := filepath.EvalSymlinks(exe); err == nil { + resolved = target + } + for _, line := range h.callLines() { + got, args, found := strings.Cut(line, "\t") + if !found { + continue + } + if same, err := filepath.EvalSymlinks(got); err == nil { + got = same + } + if got == resolved && strings.Contains(args, verb) { + return true + } + } + return false } -func (h *upgradeEnv) crwCalls() []string { return upgradeCallLines(h.t, h.calls) } -func (h *upgradeEnv) ghCallLines() []string { return upgradeCallLines(h.t, h.ghCalls) } +// recordDirs is the run directories the command left behind. +func (h *upgradeEnv) recordDirs() []string { + h.t.Helper() + entries, err := os.ReadDir(filepath.Join(h.home, "manage-state", "upgrades")) + if err != nil { + if os.IsNotExist(err) { + return nil + } + h.t.Fatal(err) + } + var out []string + for _, entry := range entries { + out = append(out, entry.Name()) + } + return out +} + +// recordJSON is the run's record.json as written, so a case can read the keys the record adds +// without depending on the struct. +func (h *upgradeEnv) recordJSON(t *testing.T) map[string]any { + t.Helper() + root := filepath.Join(h.home, "manage-state", "upgrades") + entries, err := os.ReadDir(root) + if err != nil { + t.Fatal(err) + } + if len(entries) != 1 { + t.Fatalf("the record directory holds %d entries, want 1", len(entries)) + } + data, err := os.ReadFile(filepath.Join(root, entries[0].Name(), "record.json")) + if err != nil { + t.Fatal(err) + } + var record map[string]any + if err := json.Unmarshal(data, &record); err != nil { + t.Fatal(err) + } + return record +} func (h *upgradeEnv) recordOf(t *testing.T) upgradeRecord { t.Helper() @@ -262,11 +580,113 @@ func (h *upgradeEnv) recordOf(t *testing.T) upgradeRecord { return record } -const upgradeGoodCommit = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" +// upgradeGhPathsFor answers the forge calls a released version makes: the commit ref the version +// names, and the dev-gate check runs of that commit. +func upgradeGhPathsFor(version, commit string) map[string]upgradeGhAnswer { + out := map[string]upgradeGhAnswer{} + for _, ref := range upgradeCommitRefs(version) { + out["repos/owner/repo/commits/"+ref] = upgradeGhAnswer{Body: "{\"sha\":\"" + commit + "\"}"} + } + out["repos/owner/repo/commits/"+commit+"/check-runs"] = upgradeGhAnswer{Body: "{\"check_runs\":[{\"name\":\"dev-gate\",\"conclusion\":\"success\"}]}"} + return out +} +// upgradeGhPaths answers the forge calls the harness's default version makes. func upgradeGhPaths(commit string) map[string]upgradeGhAnswer { - return map[string]upgradeGhAnswer{ - "repos/owner/repo/commits/eb2567df7": {Body: "{\"sha\":\"" + commit + "\"}"}, - "repos/owner/repo/commits/" + commit + "/check-runs": {Body: "{\"check_runs\":[{\"name\":\"dev-gate\",\"conclusion\":\"success\"}]}"}, + return upgradeGhPathsFor(upgradeFakeVersion, commit) +} + +// The command prints its usage: exit 0 for the help flags, exit 2 for anything unusable. +func TestUpgradeCommandPrintsItsUsage(t *testing.T) { + for _, tc := range []struct { + args []string + code int + }{ + {[]string{"-h"}, 0}, {[]string{"--help"}, 0}, + {[]string{"nope"}, usageExit}, {nil, usageExit}, + } { + var out, errOut strings.Builder + if code := Run(context.Background(), append([]string{"runtime-upgrade"}, tc.args...), strings.NewReader(""), &out, &errOut); code != tc.code { + t.Errorf("%q: exit %d, want %d", tc.args, code, tc.code) + } + } +} + +// A released tag version resolves through the tag ref (and its v-prefixed form); a git-describe +// version resolves through its short hash. +func TestUpgradeCommitRefs(t *testing.T) { + for _, tc := range []struct { + version string + want string + }{ + {"v0.4.0-4633-geb2567df7", "eb2567df7"}, + {"v0.4.1", "v0.4.1"}, + {"0.4.1", "0.4.1,v0.4.1"}, + {"", ""}, + } { + if got := strings.Join(upgradeCommitRefs(tc.version), ","); got != tc.want { + t.Errorf("%q: %q, want %q", tc.version, got, tc.want) + } + } +} + +// An entry through an existing symlink is refused, so a crafted archive cannot reach a file +// outside the extract directory. +func TestUpgradeExtractRefusesASymlinkEscape(t *testing.T) { + dir, outside := t.TempDir(), filepath.Join(t.TempDir(), "outside") + if err := os.WriteFile(outside, []byte("original"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outside, filepath.Join(dir, "crw")); err != nil { + t.Fatal(err) + } + archive := filepath.Join(t.TempDir(), "a.tar.gz") + if err := os.WriteFile(archive, upgradeTarGz(t, map[string]string{"crw": "overwritten"}), 0o600); err != nil { + t.Fatal(err) + } + if err := upgradeExtract(archive, dir); err == nil { + t.Fatal("an entry through a symlink was accepted") + } + if data, err := os.ReadFile(outside); err != nil || string(data) != "original" { + t.Errorf("the file outside the extract directory changed: %q %v", data, err) + } +} + +// The full success flow for both version shapes: with the pointer in place, an update that +// installs what it reports and moves the pointer to it, and a service that runs and matches, every +// step runs and the command exits 0. +func TestUpgradeSucceedsOnAHealthyHost(t *testing.T) { + for _, tc := range []struct { + name string + version string + }{{"a git-describe version", upgradeFakeVersion}, {"a released tag", "v0.4.1"}} { + t.Run(tc.name, func(t *testing.T) { + h := upgradeHarness(t, upgradeHarnessOptions{version: tc.version, gh: upgradeGhPathsFor(tc.version, upgradeGoodCommit), + pointer: true, produceRuntime: true, pointAtIt: true}) + if code := h.run("--release-dir", h.release); code != 0 { + t.Fatalf("exit %d, want 0; the record is %+v", code, h.recordOf(t)) + } + record := h.recordOf(t) + if record.Outcome != "ok" || record.Reason != "" { + t.Errorf("the record is %+v", record) + } + if len(record.Reasons) != 0 { + t.Errorf("a healthy run named the reasons %v", record.Reasons) + } + if record.StartFrom != h.installed { + t.Errorf("the restart used %q, want the runtime the update installed %q", record.StartFrom, h.installed) + } + var steps []string + for _, s := range record.Steps { + steps = append(steps, s.Step) + } + joined := strings.Join(steps, ",") + for _, want := range []string{upgradeStepSums, upgradeStepExtract, upgradeStepCommit, upgradeStepDevGate, + upgradeStepAttempts, upgradeStepSnapshot, upgradeStepStop, upgradeStepUpdate, upgradeStepStart, upgradeStepPostCheck} { + if !strings.Contains(joined, want) { + t.Errorf("the record does not name the step %q: %v", want, joined) + } + } + }) } }