From 8743be7a7b512fe505d7fd434c4786f107c34f8b Mon Sep 17 00:00:00 2001 From: codex Date: Wed, 7 Oct 2026 16:42:16 +0900 Subject: [PATCH 1/9] CRW-944: run only the verified bytes and always restart runtime-upgrade had five defects the post-merge review of PR #702 found: - it hashed the release directory's own archive and then copied that same original, so a swap between the two reads let unverified bytes run. The archive and its SHA256SUMS are now copied into the run directory first and the copy is what is verified, unpacked and executed. - it skipped the service restart when the update broke the pointer. The runtime the pointer names is recorded before the stop, and start is called after it whatever the update did, from the pointer when it still resolves and otherwise from the recorded runtime. - an update failure masked a configuration change. A configuration change now outranks an update failure in the final exit code, and the record keeps both reasons. - the post-check never compared the pointer or the version with what the update produced. It now compares the pointer with the runtime the update reported and that runtime's --version with the version the archive carried. - it read the service status once. It is now re-read every 2 seconds for up to 60 seconds, waiting for running true and matchesRunning same. The open-attempt check reads the relay doctor's contents.openAttempts instead of the store directly, and refuses when the contents cannot be read. The dry-run test that required a second run in the same second to refuse now pins Env.Now in the harness, so a wall-clock second boundary cannot decide it, and a control advances the injected clock to show the next second takes its own run directory. --- internal/manage/upgrade.go | 68 ++- internal/manage/upgrade_review702_test.go | 220 ++++++++++ internal/manage/upgrade_steps.go | 305 +++++++++---- internal/manage/upgrade_steps_test.go | 164 ++----- internal/manage/upgrade_test.go | 497 ++++++++++++++++++---- 5 files changed, 966 insertions(+), 288 deletions(-) create mode 100644 internal/manage/upgrade_review702_test.go diff --git a/internal/manage/upgrade.go b/internal/manage/upgrade.go index f4ea56a800..c54895c537 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,20 @@ type upgradeRunState struct { state string started time.Time + // digest is the verified archive's SHA-256, version 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. + digest string + version string + installed string + previous string + startFrom string + // updated reports whether the update ran and ended successfully, so the post-check knows + // whether an install target is something it has to compare the pointer with. + updated bool + beforeConfig string + reasons []string steps []upgradeStepRecord } @@ -183,7 +204,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 +232,26 @@ 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: a configuration change the +// post-check found outranks a failed update, which in turn outranks the post-check's other +// findings. 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 updateCode != 0: + return updateCode, updateReason + case post.code != 0: + return post.code, post.reason } return 0, "" } @@ -228,7 +264,9 @@ func (r *upgradeRunState) write(reason string) error { StartedAt: r.started.Format(time.RFC3339), Directory: r.dir, ExtractDir: r.extract, + StartFrom: r.startFrom, Outcome: "ok", + Reasons: r.reasons, Steps: r.steps, } if reason != "" { diff --git a/internal/manage/upgrade_review702_test.go b/internal/manage/upgrade_review702_test.go new file mode 100644 index 0000000000..ff20a5e475 --- /dev/null +++ b/internal/manage/upgrade_review702_test.go @@ -0,0 +1,220 @@ +package manage + +import ( + "bytes" + "os" + "path/filepath" + "slices" + "strings" + "testing" +) + +// 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) + } + }) +} + +// 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 +} diff --git a/internal/manage/upgrade_steps.go b/internal/manage/upgrade_steps.go index d3f04bde5a..7ad4faa373 100644 --- a/internal/manage/upgrade_steps.go +++ b/internal/manage/upgrade_steps.go @@ -16,19 +16,20 @@ import ( "path/filepath" "strings" "time" - - "github.com/thisisjun786/codex-relay-workflow/internal/relay/store" ) 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 + // upgradeServiceInterval and upgradeServiceBudget are how often, and for how long, the + // post-check reads the service status again after starting it. + upgradeServiceInterval = 2 * time.Second + upgradeServiceBudget = 60 * time.Second ) // upgradeRunCommand runs exe and returns its stdout, stderr and exit status. @@ -72,46 +73,61 @@ 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); 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)); 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.digest = strings.ToLower(got) + r.note(upgradeStepSums, nil, 0, want+" "+name, nil) + if upgradeSumsVerified != nil { + upgradeSumsVerified(r.opts.ReleaseDir) } return pinned, 0, "" } +// 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. func upgradeCopy(src, dst string) error { in, err := os.Open(src) @@ -154,7 +170,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 +186,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,42 +247,70 @@ 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"` +} + +// 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. 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) } - r.state = state - ctx, cancel := context.WithTimeout(r.ctx, upgradeStoreTimeout) - defer cancel() - read, err := store.OpenReadOnly(ctx, filepath.Join(state, "relay.sqlite3"), upgradeStoreTimeout) - if err != nil { - return r.refuse(upgradeStepAttempts, upgradeReasonStoreRead, "", err) + args := []string{"--socket", r.cfg.Relay.Socket} + if r.cfg.Relay.State != "" { + args = append([]string{"--state", r.cfg.Relay.State}, args...) } - 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) + args = append(args, "doctor") + out, code, err := r.command(r.ctx, upgradeCommandTimeout, upgradeStepAttempts, + filepath.Join(pointer, "bin", "codex-session-relay"), args...) + if err != nil || code != 0 { return upgradeExitRefused, upgradeReasonStoreRead } - r.note(upgradeStepAttempts, []string{query}, 0, fmt.Sprintf("%d", open), nil) - if open != 0 { + answer, err := upgradeParseDoctorAnswer(out) + if err != nil { + return r.refuse(upgradeStepAttempts, upgradeReasonStoreRead, out, err) + } + state := r.cfg.Relay.State + 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")) + } + 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 - } - state, err := r.e.relayHelperDoctorState(r.ctx, r.cfg.Relay.Socket) - return state, err -} - // snapshot is step 5: what the run must be able to compare afterwards. func (r *upgradeRunState) snapshot() (int, string) { pointer, err := upgradePointerTarget(r.e) @@ -283,13 +330,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,7 +349,11 @@ 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 && code == 0 { + r.updated = true + r.installed = upgradeInstalledRuntime(out) + } if err != nil { return upgradeExitUpdateFailed, upgradeReasonUpdateFailed } @@ -310,47 +363,133 @@ func (r *upgradeRunState) stopAndUpdate() (int, string) { 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 used when it +// still resolves; otherwise the runtime the pointer named before the stop, which is what the +// service was running, so a pointer the update broke cannot leave the service down. Which runtime +// was used is written into the record. func (r *upgradeRunState) start() { - pointer, err := upgradePointerTarget(r.e) - if err != nil { + runtime, from := "", "" + if pointer, err := upgradePointerTarget(r.e); err == nil { + runtime, from = pointer, "the pointer" + } else if r.previous != "" { + runtime, from = r.previous, "the runtime the pointer named before the stop" + } else { r.note(upgradeStepStart, nil, 1, "", err) return } + r.startFrom = runtime // 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") + 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:]...) + r.note(upgradeStepStart, argv, code, "started from "+from+": "+runtime+"\n"+out+stderr, err) +} + +// 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 } -// 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) { +// 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, the service must come up running and matching +// within the wait budget, and the configuration file must be unchanged. +func (r *upgradeRunState) postCheck() upgradePostCheck { + var post upgradePostCheck pointer, err := upgradePointerTarget(r.e) - if err != nil { + switch { + case err != nil: r.note(upgradeStepPostCheck, nil, 1, "", err) - return upgradeExitPostCheck, upgradeReasonPostCheck - } - 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 + post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) + case r.updated && 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")) + post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) + case r.updated && !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)) + post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) + default: + 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)) + post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) + } } - 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 + runtime := r.startFrom + if runtime == "" { + 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) } - digest, err := upgradeFileDigest(upgradeConfigPath(r.e)) - if err != nil || digest != r.beforeConfig { + if digest, err := upgradeFileDigest(upgradeConfigPath(r.e)); err != nil || 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 +} + +// 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 +509,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 68d69f1f5e..b694efad17 100644 --- a/internal/manage/upgrade_test.go +++ b/internal/manage/upgrade_test.go @@ -12,32 +12,93 @@ import ( "fmt" "os" "path/filepath" + "strconv" "strings" "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 + 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 + + // 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 + // 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 + 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 + // 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 + // 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 { @@ -47,6 +108,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") @@ -61,40 +125,113 @@ 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"), + pointerLink: filepath.Join(root, "current"), + previous: filepath.Join(root, "bin-"+upgradeArchiveVersion+"-000000000000"), + installed: filepath.Join(root, "bin-"+upgradeArchiveVersion+"-111111111111"), + now: started} + h.script = h.fakeScript(opts) h.writeFakes(opts) + h.writeStore() + h.writeArchive(opts) + h.installRuntime(h.previous, opts.version) 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}}" + } + 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 status\"*)\n") + b.WriteString(" n=$(cat " + coreShellQuote(h.statusCounter) + " 2>/dev/null || printf '0')\n") + b.WriteString(" n=$((n + 1))\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*) printf '%s\\n' " + coreShellQuote(doctor) + " ;;\n") + b.WriteString("*\"install update\"*)\n") + if opts.breakPointer { + b.WriteString(" rm -f " + coreShellQuote(h.pointerLink) + "\n") + } + if opts.mutateConfig { + b.WriteString(" printf 'changed\\n' >> \"$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 { @@ -119,7 +256,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 { @@ -129,47 +284,31 @@ 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" - if err := os.WriteFile(filepath.Join(bin, "crw"), []byte(crw), 0o700); err != nil { + if err := os.WriteFile(filepath.Join(bin, "crw"), []byte(h.script), 0o700); err != nil { + h.t.Fatal(err) + } + 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") if err := os.WriteFile(filepath.Join(bin, "gh"), []byte(body.String()), 0o700); err != nil { @@ -178,31 +317,38 @@ 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"} { - if err := os.WriteFile(filepath.Join(target, "bin", name), body, 0o700); err != nil { + if err := os.WriteFile(filepath.Join(bin, name), []byte(h.script), 0o700); err != nil { h.t.Fatal(err) } } - 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 @@ -216,20 +362,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) } -func upgradeCallLines(t *testing.T, path string) []string { +// 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 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() @@ -252,11 +487,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) + } + } + }) } } From d44f17e35568c32fa2689dcc3f0e5625d7b57999 Mon Sep 17 00:00:00 2001 From: codex Date: Wed, 7 Oct 2026 17:54:53 +0900 Subject: [PATCH 2/9] CRW-944: fall back when the promoted runtime will not start The first external review of this pull request found five gaps in the new behaviour. Each has a test that fails on the previous head: - a pointer can resolve to a directory the service cannot be started from. The restart now tries the pointer's runtime and falls back to the runtime the pointer named before the stop when that start does not succeed, and the record names the runtime the service came back on. - a configuration file the post-check could not read is not a change: it is recorded as a post-check finding, so it no longer takes the exit code from an update failure that would otherwise report it. - every failure names its reasons, including a run that refused before the update, which previously had only the single reason field. - 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, so the version comparison runs only against a runtime the update put in service. - the installer's exit 3 is a promotion whose claim did not settle: the runtime is in service, so the pointer and version are compared with it even though the run is reported as failed. --- internal/manage/upgrade.go | 16 +++- internal/manage/upgrade_review702_test.go | 100 ++++++++++++++++++++++ internal/manage/upgrade_steps.go | 97 +++++++++++++++------ internal/manage/upgrade_test.go | 32 ++++++- 4 files changed, 213 insertions(+), 32 deletions(-) diff --git a/internal/manage/upgrade.go b/internal/manage/upgrade.go index c54895c537..479e065bea 100644 --- a/internal/manage/upgrade.go +++ b/internal/manage/upgrade.go @@ -177,9 +177,11 @@ type upgradeRunState struct { installed string previous string startFrom string - // updated reports whether the update ran and ended successfully, so the post-check knows - // whether an install target is something it has to compare the pointer with. - updated 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 @@ -266,9 +268,15 @@ func (r *upgradeRunState) write(reason string) error { ExtractDir: r.extract, StartFrom: r.startFrom, Outcome: "ok", - Reasons: r.reasons, 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 index ff20a5e475..d785cc4be8 100644 --- a/internal/manage/upgrade_review702_test.go +++ b/internal/manage/upgrade_review702_test.go @@ -218,3 +218,103 @@ func upgradeReview702StatusReads(h *upgradeEnv) int { } 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. +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 != 0 { + t.Fatalf("exit %d, want 0; the service came back on the previous runtime, so the run is a success: %+v", code, h.recordOf(t)) + } + 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) + } +} + +// TestUpgradeReview702UnreadableConfigIsNotAChange: a configuration file the post-check cannot read +// is not a configuration change. An update failure must keep 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 != 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 does not name the runtime the update promoted, so it is a mismatch: %v", got) + } + }) +} diff --git a/internal/manage/upgrade_steps.go b/internal/manage/upgrade_steps.go index 7ad4faa373..9c7e77cf54 100644 --- a/internal/manage/upgrade_steps.go +++ b/internal/manage/upgrade_steps.go @@ -14,6 +14,7 @@ import ( "os" "os/exec" "path/filepath" + "slices" "strings" "time" ) @@ -30,6 +31,13 @@ const ( // post-check reads the service status again after starting it. upgradeServiceInterval = 2 * time.Second upgradeServiceBudget = 60 * time.Second + // 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 ) // upgradeRunCommand runs exe and returns its stdout, stderr and exit status. @@ -350,13 +358,17 @@ func (r *upgradeRunState) stopAndUpdate() (int, string) { args = append(args, "--issue", r.opts.Issue) } out, code, err := r.command(r.ctx, upgradeInstallTimeout, upgradeStepUpdate, filepath.Join(r.extract, "crw"), args...) - if err == nil && code == 0 { - r.updated = true - r.installed = upgradeInstalledRuntime(out) - } 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 } @@ -376,28 +388,46 @@ func upgradeInstalledRuntime(answer string) string { return reported.Environment } -// start is step 7: start again, whatever the update did. The pointer's runtime is used when it -// still resolves; otherwise the runtime the pointer named before the stop, which is what the -// service was running, so a pointer the update broke cannot leave the service down. Which runtime -// was used is written into the record. +// 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 came back on. func (r *upgradeRunState) start() { - runtime, from := "", "" + var candidates []string if pointer, err := upgradePointerTarget(r.e); err == nil { - runtime, from = pointer, "the pointer" + candidates = append(candidates, pointer) } else if r.previous != "" { - runtime, from = r.previous, "the runtime the pointer named before the stop" + r.note(upgradeStepStart, nil, 1, "", err) } else { r.note(upgradeStepStart, nil, 1, "", err) return } - r.startFrom = runtime - // 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() - 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:]...) - r.note(upgradeStepStart, argv, code, "started from "+from+": "+runtime+"\n"+out+stderr, err) + 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 + return + } + if i+1 == len(candidates) { + // Nothing started the service. startFrom names the last runtime tried, so the post-check + // reads the status the way this attempt left it. + r.startFrom = runtime + } + } } // upgradePostCheck is what step 8 established: the status and reason it reports, every reason it @@ -419,18 +449,23 @@ func (r *upgradeRunState) postCheck() upgradePostCheck { case err != nil: r.note(upgradeStepPostCheck, nil, 1, "", err) post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) - case r.updated && r.installed == "": + 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")) post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) - case r.updated && !upgradeSameDirectory(pointer, r.installed): + 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)) post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) default: - 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)) - post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) + // 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)) + post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) + } } } runtime := r.startFrom @@ -440,7 +475,15 @@ func (r *upgradeRunState) postCheck() upgradePostCheck { if runtime != "" && !r.waitForService(runtime) { post.reasons = append(post.reasons, upgradeReasonPostCheck) } - if digest, err := upgradeFileDigest(upgradeConfigPath(r.e)); err != nil || digest != r.beforeConfig { + // A configuration file the post-check could not read is not a configuration change: only a + // digest that was read and differs is. An unreadable one is a post-check finding of its own, so + // it never takes the exit code away from an update failure that would otherwise report it. + digest, err := upgradeFileDigest(upgradeConfigPath(r.e)) + switch { + 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")) post.reasons = append(post.reasons, upgradeReasonConfigChanged) post.configChanged = true diff --git a/internal/manage/upgrade_test.go b/internal/manage/upgrade_test.go index b694efad17..1bcebfc5bc 100644 --- a/internal/manage/upgrade_test.go +++ b/internal/manage/upgrade_test.go @@ -58,6 +58,9 @@ type upgradeEnv struct { 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 @@ -76,6 +79,10 @@ type upgradeHarnessOptions struct { // 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 // doctorUnavailable makes the doctor answer report contents it could not read. @@ -94,6 +101,11 @@ type upgradeHarnessOptions struct { breakPointer bool // mutateConfig makes the fake update append to the configuration file. mutateConfig bool + // unreadableConfigAfter makes the fake update leave the configuration file unreadable. + unreadableConfigAfter bool + // breakInstalledStart makes the runtime the update produces fail to start the service, so the + // restart has to fall back. + breakInstalledStart 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 @@ -135,6 +147,7 @@ func upgradeHarness(t *testing.T, opts upgradeHarnessOptions) *upgradeEnv { 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"), @@ -143,7 +156,11 @@ func upgradeHarness(t *testing.T, opts upgradeHarnessOptions) *upgradeEnv { h.writeFakes(opts) h.writeStore() h.writeArchive(opts) - h.installRuntime(h.previous, opts.version) + previous := opts.previousVersion + if previous == "" { + previous = opts.version + } + h.installRuntime(h.previous, previous) h.installPointer(opts.pointer) return h } @@ -191,9 +208,19 @@ func (h *upgradeEnv) fakeScript(opts upgradeHarnessOptions) string { 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") + } + 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") + // 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") @@ -207,6 +234,9 @@ func (h *upgradeEnv) fakeScript(opts upgradeHarnessOptions) string { if opts.mutateConfig { b.WriteString(" printf 'changed\\n' >> \"$CODEX_HOME/config.toml\"\n") } + if opts.unreadableConfigAfter { + 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") From 4e06ab0ced34e2079e5fcc6596186835ccc8f0b0 Mon Sep 17 00:00:00 2001 From: codex Date: Wed, 7 Oct 2026 18:09:28 +0900 Subject: [PATCH 3/9] CRW-944: gofmt the upgrade harness --- internal/manage/upgrade_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/manage/upgrade_test.go b/internal/manage/upgrade_test.go index 1bcebfc5bc..acbdc3aed7 100644 --- a/internal/manage/upgrade_test.go +++ b/internal/manage/upgrade_test.go @@ -61,7 +61,7 @@ type upgradeEnv struct { // 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 + 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. From 27cb1d916b2ee3341da01ffb8018ea8217902c34 Mon Sep 17 00:00:00 2001 From: codex Date: Wed, 7 Oct 2026 19:57:56 +0900 Subject: [PATCH 4/9] CRW-944: drop the unread digest field and cover both fail-closed guards The independent review of the previous head found two defects in what this change adds: - the run state kept the verified archive's digest and never read it; the sums step already records it in the run record, so the field is gone. - the open-attempt guard refuses both unreadable contents and readable contents that carry no count, and only the first half had a case. The second half now has one, so a regression that read a missing count as zero fails. The duplicated installer statuses stay: the constants name the coupling in a comment, and importing the installer's own constants would add a package dependency this change does not need. --- internal/manage/upgrade.go | 8 ++++---- internal/manage/upgrade_review702_test.go | 10 ++++++++++ internal/manage/upgrade_steps.go | 1 - internal/manage/upgrade_test.go | 8 +++++++- 4 files changed, 21 insertions(+), 6 deletions(-) diff --git a/internal/manage/upgrade.go b/internal/manage/upgrade.go index 479e065bea..71ce5c9044 100644 --- a/internal/manage/upgrade.go +++ b/internal/manage/upgrade.go @@ -169,10 +169,10 @@ type upgradeRunState struct { state string started time.Time - // digest is the verified archive's SHA-256, version 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. - digest string + // 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 diff --git a/internal/manage/upgrade_review702_test.go b/internal/manage/upgrade_review702_test.go index d785cc4be8..a0bf22184c 100644 --- a/internal/manage/upgrade_review702_test.go +++ b/internal/manage/upgrade_review702_test.go @@ -206,6 +206,16 @@ func TestUpgradeReview702OpenAttemptsFromDoctor(t *testing.T) { 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. diff --git a/internal/manage/upgrade_steps.go b/internal/manage/upgrade_steps.go index 9c7e77cf54..03e6c68cff 100644 --- a/internal/manage/upgrade_steps.go +++ b/internal/manage/upgrade_steps.go @@ -120,7 +120,6 @@ func (r *upgradeRunState) verifySums() (string, int, string) { r.note(upgradeStepSums, nil, 1, "", fmt.Errorf("%s does not match %s", name, upgradeSumsName)) return "", upgradeExitRefused, upgradeReasonSumsFailed } - r.digest = strings.ToLower(got) r.note(upgradeStepSums, nil, 0, want+" "+name, nil) if upgradeSumsVerified != nil { upgradeSumsVerified(r.opts.ReleaseDir) diff --git a/internal/manage/upgrade_test.go b/internal/manage/upgrade_test.go index acbdc3aed7..0d3e72de96 100644 --- a/internal/manage/upgrade_test.go +++ b/internal/manage/upgrade_test.go @@ -87,7 +87,10 @@ type upgradeHarnessOptions struct { openAttempts int // doctorUnavailable makes the doctor answer report contents it could not read. doctorUnavailable bool - gh map[string]upgradeGhAnswer + // 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 + 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. @@ -191,6 +194,9 @@ func (h *upgradeEnv) fakeScript(opts upgradeHarnessOptions) string { 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}}" + } answers := opts.statusAnswers if len(answers) == 0 { answers = []string{upgradeStatusSame} From bb7ad7163a08e182673699080c1a4d1a0dac1a7c Mon Sep 17 00:00:00 2001 From: crw-934 Date: Wed, 7 Oct 2026 22:00:37 +0900 Subject: [PATCH 5/9] CRW-944: report the mismatch when the service runs another runtime The pre-merge evaluation of the reviewed head found three in-scope defects. The restart's fallback kept the relay up when the runtime the update installed would not start, but the run still reported success: the post-check compared the pointer with the installed runtime and then read the service status from the runtime the restart actually used, without requiring the two to be the same. A pointer on the new runtime with the service running the old one is now a runtime_mismatch. The post-check treated every read failure of the configuration file as no change. A file the snapshot read and that is now gone is a change the run can state exactly, so it is config_changed; a file that still exists but cannot be read stays a post-check finding of its own. A pointer that does not name the runtime the update promoted is a mismatch even when the update itself failed, so the mismatch exit code is no longer hidden by the update failure. --- internal/manage/upgrade.go | 11 ++++-- internal/manage/upgrade_review702_test.go | 45 +++++++++++++++++++---- internal/manage/upgrade_steps.go | 27 ++++++++++++-- internal/manage/upgrade_test.go | 9 ++++- 4 files changed, 76 insertions(+), 16 deletions(-) diff --git a/internal/manage/upgrade.go b/internal/manage/upgrade.go index 71ce5c9044..a8ddc224ce 100644 --- a/internal/manage/upgrade.go +++ b/internal/manage/upgrade.go @@ -238,10 +238,11 @@ func (r *upgradeRunState) execute() (int, string) { return r.outcome(updateCode, updateReason, post) } -// outcome is the run's final status and reason, in the decided order: a configuration change the -// post-check found outranks a failed update, which in turn outranks the post-check's other -// findings. Every reason that applied is kept in the record, so a run that both failed to update -// and changed the configuration names both. +// 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 { r.reasons = append(r.reasons, updateReason) @@ -250,6 +251,8 @@ func (r *upgradeRunState) outcome(updateCode int, updateReason string, post upgr switch { case post.configChanged: return upgradeExitPostCheck, upgradeReasonConfigChanged + case post.mismatch: + return upgradeExitPostCheck, upgradeReasonRuntimeMismatch case updateCode != 0: return updateCode, updateReason case post.code != 0: diff --git a/internal/manage/upgrade_review702_test.go b/internal/manage/upgrade_review702_test.go index a0bf22184c..ff9e2fbdae 100644 --- a/internal/manage/upgrade_review702_test.go +++ b/internal/manage/upgrade_review702_test.go @@ -233,12 +233,17 @@ func upgradeReview702StatusReads(h *upgradeEnv) int { // 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. +// 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 != 0 { - t.Fatalf("exit %d, want 0; the service came back on the previous runtime, so the run is a success: %+v", code, h.recordOf(t)) + 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()) @@ -251,9 +256,30 @@ func TestUpgradeReview702RestartsFromThePreviousRuntimeWhenTheNewOneWillNotStart } } -// TestUpgradeReview702UnreadableConfigIsNotAChange: a configuration file the post-check cannot read -// is not a configuration change. An update failure must keep its own exit code, and the record must -// not claim config_changed. +// 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}) @@ -320,8 +346,11 @@ func TestUpgradeReview702IncompletePromotionIsChecked(t *testing.T) { 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 != upgradeExitUpdateFailed { - t.Fatalf("exit %d, want %d; the record is %+v", code, upgradeExitUpdateFailed, h.recordOf(t)) + 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) diff --git a/internal/manage/upgrade_steps.go b/internal/manage/upgrade_steps.go index 03e6c68cff..3ae5c02a22 100644 --- a/internal/manage/upgrade_steps.go +++ b/internal/manage/upgrade_steps.go @@ -436,6 +436,11 @@ type upgradePostCheck struct { 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 @@ -448,12 +453,23 @@ func (r *upgradeRunState) postCheck() upgradePostCheck { case err != nil: r.note(upgradeStepPostCheck, nil, 1, "", err) post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) + post.mismatch = true 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")) post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) + post.mismatch = true 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)) post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) + post.mismatch = true + case r.promoted && r.startFrom != "" && !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)) + post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) + post.mismatch = true 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 @@ -464,6 +480,7 @@ func (r *upgradeRunState) postCheck() upgradePostCheck { 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)) post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) + post.mismatch = true } } } @@ -474,11 +491,15 @@ func (r *upgradeRunState) postCheck() upgradePostCheck { if runtime != "" && !r.waitForService(runtime) { post.reasons = append(post.reasons, upgradeReasonPostCheck) } - // A configuration file the post-check could not read is not a configuration change: only a - // digest that was read and differs is. An unreadable one is a post-check finding of its own, so - // it never takes the exit code away from an update failure that would otherwise report it. + // 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)) 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) diff --git a/internal/manage/upgrade_test.go b/internal/manage/upgrade_test.go index 0d3e72de96..59ac7d2c89 100644 --- a/internal/manage/upgrade_test.go +++ b/internal/manage/upgrade_test.go @@ -104,8 +104,12 @@ type upgradeHarnessOptions struct { breakPointer bool // mutateConfig makes the fake update append to the configuration file. mutateConfig bool - // unreadableConfigAfter makes the fake update leave the configuration file unreadable. + // 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 @@ -241,6 +245,9 @@ func (h *upgradeEnv) fakeScript(opts upgradeHarnessOptions) string { 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 { From 920010ebdf49232f42e36afb3d941a04f634c070 Mon Sep 17 00:00:00 2001 From: crw-934 Date: Wed, 7 Oct 2026 22:39:55 +0900 Subject: [PATCH 6/9] CRW-944: name the runtime the service is really on The pre-merge evaluation of the first correction round found two more in-scope defects, both in the restart and post-check. The restart treated a start answer of already_running as a failed launch and fell back to the previous runtime, recording that runtime as the one the service was on even when the service was up on the runtime the update installed. A service already running holds the lock, so no other runtime can start either; already_running now names the runtime the service is on, and the post-check compares that runtime with the installed one. The post-check assumed a failed update had rolled back without checking the pointer still named the runtime the update replaced. An update that puts no runtime in service is a rollback only while the pointer names the previous runtime; a pointer left on any other runtime is a mismatch. --- internal/manage/upgrade.go | 4 +++ internal/manage/upgrade_review702_test.go | 37 +++++++++++++++++++++ internal/manage/upgrade_steps.go | 40 ++++++++++++++++++++--- internal/manage/upgrade_test.go | 22 +++++++++++++ 4 files changed, 99 insertions(+), 4 deletions(-) diff --git a/internal/manage/upgrade.go b/internal/manage/upgrade.go index a8ddc224ce..43df0f493c 100644 --- a/internal/manage/upgrade.go +++ b/internal/manage/upgrade.go @@ -177,6 +177,10 @@ type upgradeRunState struct { 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 diff --git a/internal/manage/upgrade_review702_test.go b/internal/manage/upgrade_review702_test.go index ff9e2fbdae..8cb4fa64a0 100644 --- a/internal/manage/upgrade_review702_test.go +++ b/internal/manage/upgrade_review702_test.go @@ -357,3 +357,40 @@ func TestUpgradeReview702IncompletePromotionIsChecked(t *testing.T) { } }) } + +// 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) + } +} diff --git a/internal/manage/upgrade_steps.go b/internal/manage/upgrade_steps.go index 3ae5c02a22..e726ad8bd3 100644 --- a/internal/manage/upgrade_steps.go +++ b/internal/manage/upgrade_steps.go @@ -391,7 +391,10 @@ func upgradeInstalledRuntime(answer string) string { // 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 came back on. +// 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() { var candidates []string if pointer, err := upgradePointerTarget(r.e); err == nil { @@ -419,6 +422,12 @@ func (r *upgradeRunState) start() { 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) { @@ -429,6 +438,19 @@ func (r *upgradeRunState) start() { } } +// 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"` + } + if json.Unmarshal([]byte(out), &answer) != nil { + return false + } + 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 { @@ -444,8 +466,10 @@ type upgradePostCheck struct { } // 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, the service must come up running and matching -// within the wait budget, and the configuration file must be unchanged. +// 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. func (r *upgradeRunState) postCheck() upgradePostCheck { var post upgradePostCheck pointer, err := upgradePointerTarget(r.e) @@ -462,7 +486,7 @@ func (r *upgradeRunState) postCheck() upgradePostCheck { r.note(upgradeStepPostCheck, nil, 1, "", fmt.Errorf("the runtime pointer names %s, not %s, the runtime the update installed", pointer, r.installed)) post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) post.mismatch = true - case r.promoted && r.startFrom != "" && !upgradeSameDirectory(r.startFrom, r.installed): + 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 @@ -470,6 +494,14 @@ func (r *upgradeRunState) postCheck() upgradePostCheck { r.note(upgradeStepPostCheck, nil, 1, "", fmt.Errorf("the service came back on %s, not %s, the runtime the update installed", r.startFrom, r.installed)) post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) post.mismatch = true + 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)) + post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) + post.mismatch = true 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 diff --git a/internal/manage/upgrade_test.go b/internal/manage/upgrade_test.go index 59ac7d2c89..543647b14e 100644 --- a/internal/manage/upgrade_test.go +++ b/internal/manage/upgrade_test.go @@ -67,6 +67,9 @@ type upgradeEnv struct { // 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 @@ -99,6 +102,10 @@ type upgradeHarnessOptions struct { 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 @@ -113,6 +120,10 @@ type upgradeHarnessOptions struct { // breakInstalledStart makes the runtime the update produces fail to start the service, so the // restart has to fall back. breakInstalledStart 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 @@ -158,6 +169,7 @@ func upgradeHarness(t *testing.T, opts upgradeHarnessOptions) *upgradeEnv { 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) @@ -168,6 +180,7 @@ func upgradeHarness(t *testing.T, opts upgradeHarnessOptions) *upgradeEnv { previous = opts.version } h.installRuntime(h.previous, previous) + h.installRuntime(h.other, previous) h.installPointer(opts.pointer) return h } @@ -223,6 +236,12 @@ func (h *upgradeEnv) fakeScript(opts upgradeHarnessOptions) string { // 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") @@ -241,6 +260,9 @@ func (h *upgradeEnv) fakeScript(opts upgradeHarnessOptions) string { 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") } From 6b36c52bc5470076015698c1b8779dc8b2e1fe1e Mon Sep 17 00:00:00 2001 From: thisisjun786 <259586770+thisisjun786@users.noreply.github.com> Date: Wed, 7 Oct 2026 23:20:13 +0900 Subject: [PATCH 7/9] CRW-944: verify the install target again after the service wait The pre-merge evaluation found that the pointer and version were compared only before the service wait. 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 then reported success with the pointer no longer naming the installed runtime. The install-target check now runs before and after the wait, and a mismatch it finds is recorded once. A start that never leaves the service up no longer records the last runtime it tried as the runtime the service came back on. --- internal/manage/upgrade_review702_test.go | 38 ++++++++ internal/manage/upgrade_steps.go | 104 ++++++++++++---------- internal/manage/upgrade_test.go | 6 ++ 3 files changed, 103 insertions(+), 45 deletions(-) diff --git a/internal/manage/upgrade_review702_test.go b/internal/manage/upgrade_review702_test.go index 8cb4fa64a0..5051267b65 100644 --- a/internal/manage/upgrade_review702_test.go +++ b/internal/manage/upgrade_review702_test.go @@ -2,6 +2,7 @@ package manage import ( "bytes" + "context" "os" "path/filepath" "slices" @@ -394,3 +395,40 @@ func TestUpgradeReview702PointerMovedToAThirdRuntimeIsAMismatch(t *testing.T) { 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) + } +} + +// TestUpgradeReview702ServiceWaitBudgetIsRecorded: a service that never reports itself running and +// matching is a post-check failure, and the wait records the last answer it read rather than +// returning quietly. The budget is measured on the wall clock, so the case is driven with a context +// that has already ended: the wait must stop, record and report failure rather than hang or pass. +func TestUpgradeReview702ServiceWaitBudgetIsRecorded(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + r := &upgradeRunState{ctx: ctx, cfg: &Config{}, state: "/state"} + r.cfg.Relay.Socket = "/socket" + if r.waitForService(t.TempDir()) { + t.Fatal("a service that never reported itself running and matching was reported as up") + } + if len(r.steps) == 0 { + t.Fatal("the wait recorded nothing") + } + last := r.steps[len(r.steps)-1] + if last.Step != upgradeStepPostCheck || last.Exit == 0 { + t.Errorf("the wait's last step is %+v, want a failed %s", last, upgradeStepPostCheck) + } +} diff --git a/internal/manage/upgrade_steps.go b/internal/manage/upgrade_steps.go index e726ad8bd3..c42e33b3f9 100644 --- a/internal/manage/upgrade_steps.go +++ b/internal/manage/upgrade_steps.go @@ -431,9 +431,10 @@ func (r *upgradeRunState) start() { return } if i+1 == len(candidates) { - // Nothing started the service. startFrom names the last runtime tried, so the post-check - // reads the status the way this attempt left it. - r.startFrom = runtime + // 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)) } } } @@ -469,39 +470,75 @@ type upgradePostCheck struct { // 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. +// 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 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)) + 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")) + 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) - post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) - post.mismatch = true + 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")) - post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) - post.mismatch = true + 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)) - post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) - post.mismatch = true + 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)) - post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) - post.mismatch = true + 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)) - post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) - post.mismatch = true + 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 @@ -511,41 +548,18 @@ func (r *upgradeRunState) postCheck() upgradePostCheck { 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)) - post.reasons = append(post.reasons, upgradeReasonRuntimeMismatch) - post.mismatch = true + r.mismatch(post) } } } - runtime := r.startFrom - if runtime == "" { - runtime = pointer - } - if runtime != "" && !r.waitForService(runtime) { - post.reasons = append(post.reasons, upgradeReasonPostCheck) - } - // 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)) - 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")) - post.reasons = append(post.reasons, upgradeReasonConfigChanged) - post.configChanged = true - } - if len(post.reasons) == 0 { - return 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) } - post.code = upgradeExitPostCheck - post.reason = post.reasons[0] - return post } // waitForService reads the service status again every upgradeServiceInterval until it reports the diff --git a/internal/manage/upgrade_test.go b/internal/manage/upgrade_test.go index 543647b14e..c8581d7f20 100644 --- a/internal/manage/upgrade_test.go +++ b/internal/manage/upgrade_test.go @@ -120,6 +120,9 @@ type upgradeHarnessOptions struct { // 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. @@ -247,6 +250,9 @@ func (h *upgradeEnv) fakeScript(opts upgradeHarnessOptions) string { 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") From e5e0913d355ee4054136cd3f7c12991ede8dbbcf Mon Sep 17 00:00:00 2001 From: thisisjun786 <259586770+thisisjun786@users.noreply.github.com> Date: Thu, 8 Oct 2026 00:01:56 +0900 Subject: [PATCH 8/9] CRW-944: read open attempts from the store the service actually serves The doctor names the store the relay service serves in serviceStore when discovery selected another directory; the check ignored it and judged the selected directory's contents, so a served store with open attempts could pass as none. The served store is now asked for its own answer, and an unreadable served store is refused rather than read as empty. --- internal/manage/upgrade_review702_test.go | 27 +++++++++++ internal/manage/upgrade_steps.go | 57 +++++++++++++++++++---- internal/manage/upgrade_test.go | 16 ++++++- 3 files changed, 88 insertions(+), 12 deletions(-) diff --git a/internal/manage/upgrade_review702_test.go b/internal/manage/upgrade_review702_test.go index 5051267b65..873b98e90f 100644 --- a/internal/manage/upgrade_review702_test.go +++ b/internal/manage/upgrade_review702_test.go @@ -432,3 +432,30 @@ func TestUpgradeReview702ServiceWaitBudgetIsRecorded(t *testing.T) { t.Errorf("the wait's last step is %+v, want a failed %s", last, upgradeStepPostCheck) } } + +// 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()) + } + }) +} diff --git a/internal/manage/upgrade_steps.go b/internal/manage/upgrade_steps.go index c42e33b3f9..8dd5ed82fd 100644 --- a/internal/manage/upgrade_steps.go +++ b/internal/manage/upgrade_steps.go @@ -264,6 +264,14 @@ type upgradeDoctorAnswer 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 @@ -279,7 +287,10 @@ func upgradeParseDoctorAnswer(out string) (upgradeDoctorAnswer, error) { // 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. +// 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) { pointer, err := upgradePointerTarget(r.e) if err != nil { @@ -287,21 +298,31 @@ func (r *upgradeRunState) checkOpenAttempts() (int, string) { // snapshot reported for this host before this step moved ahead of it. return r.refuse(upgradeStepAttempts, upgradeReasonPointer, "", err) } - args := []string{"--socket", r.cfg.Relay.Socket} - if r.cfg.Relay.State != "" { - args = append([]string{"--state", r.cfg.Relay.State}, args...) - } - args = append(args, "doctor") - out, code, err := r.command(r.ctx, upgradeCommandTimeout, upgradeStepAttempts, - filepath.Join(pointer, "bin", "codex-session-relay"), args...) + 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 } - answer, err := upgradeParseDoctorAnswer(out) + 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, out, err) } - state := r.cfg.Relay.State if state == "" { state = answer.StateSelection.Path } @@ -318,6 +339,22 @@ func (r *upgradeRunState) checkOpenAttempts() (int, string) { return 0, "" } +// 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 + } + answer, err := upgradeParseDoctorAnswer(out) + return answer, out, code, err +} + // snapshot is step 5: what the run must be able to compare afterwards. func (r *upgradeRunState) snapshot() (int, string) { pointer, err := upgradePointerTarget(r.e) diff --git a/internal/manage/upgrade_test.go b/internal/manage/upgrade_test.go index c8581d7f20..8c85f1ea17 100644 --- a/internal/manage/upgrade_test.go +++ b/internal/manage/upgrade_test.go @@ -93,7 +93,13 @@ type upgradeHarnessOptions struct { // 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 - gh map[string]upgradeGhAnswer + // 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. @@ -217,6 +223,11 @@ func (h *upgradeEnv) fakeScript(opts upgradeHarnessOptions) string { 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} @@ -261,7 +272,8 @@ func (h *upgradeEnv) fakeScript(opts upgradeHarnessOptions) string { 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*) printf '%s\\n' " + coreShellQuote(doctor) + " ;;\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") From 76a5979177cec665c5b16628a83e448138f9c7fd Mon Sep 17 00:00:00 2001 From: thisisjun786 <259586770+thisisjun786@users.noreply.github.com> Date: Thu, 8 Oct 2026 00:46:53 +0900 Subject: [PATCH 9/9] CRW-944: bound the pre-verification copy and pin the wait budget The pre-verification copy read whatever it was given, so a release directory holding something far larger than the archive could fill the state disk the live relay shares before the checksum even ran. The copy now reads at most the installer's own input limit and removes its partial file when it refuses or fails. The wait-budget test now shrinks the budget and drives a service that answers but never settles, so it spends the budget and checks the last answer is recorded; the earlier form only proved the parent context ended. --- internal/manage/upgrade_review702_test.go | 73 ++++++++++++++++------- internal/manage/upgrade_steps.go | 49 +++++++++++---- 2 files changed, 89 insertions(+), 33 deletions(-) diff --git a/internal/manage/upgrade_review702_test.go b/internal/manage/upgrade_review702_test.go index 873b98e90f..4d0615393b 100644 --- a/internal/manage/upgrade_review702_test.go +++ b/internal/manage/upgrade_review702_test.go @@ -2,12 +2,12 @@ package manage import ( "bytes" - "context" "os" "path/filepath" "slices" "strings" "testing" + "time" ) // The review-702 cases: the defects the post-merge review of PR #702 found in @@ -412,27 +412,6 @@ func TestUpgradeReview702PointerBrokenDuringTheWaitIsAMismatch(t *testing.T) { } } -// TestUpgradeReview702ServiceWaitBudgetIsRecorded: a service that never reports itself running and -// matching is a post-check failure, and the wait records the last answer it read rather than -// returning quietly. The budget is measured on the wall clock, so the case is driven with a context -// that has already ended: the wait must stop, record and report failure rather than hang or pass. -func TestUpgradeReview702ServiceWaitBudgetIsRecorded(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - cancel() - r := &upgradeRunState{ctx: ctx, cfg: &Config{}, state: "/state"} - r.cfg.Relay.Socket = "/socket" - if r.waitForService(t.TempDir()) { - t.Fatal("a service that never reported itself running and matching was reported as up") - } - if len(r.steps) == 0 { - t.Fatal("the wait recorded nothing") - } - last := r.steps[len(r.steps)-1] - if last.Step != upgradeStepPostCheck || last.Exit == 0 { - t.Errorf("the wait's last step is %+v, want a failed %s", last, upgradeStepPostCheck) - } -} - // 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 @@ -459,3 +438,53 @@ func TestUpgradeReview702ServedStoreOpenAttempts(t *testing.T) { } }) } + +// 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 8dd5ed82fd..16c593e1f1 100644 --- a/internal/manage/upgrade_steps.go +++ b/internal/manage/upgrade_steps.go @@ -17,6 +17,8 @@ import ( "slices" "strings" "time" + + "github.com/thisisjun786/codex-relay-workflow/internal/runtime/install" ) const ( @@ -27,10 +29,6 @@ const ( // which exercises the bridge and may copy a large relay state. upgradeCommandTimeout = 60 * time.Second upgradeInstallTimeout = 30 * time.Minute - // upgradeServiceInterval and upgradeServiceBudget are how often, and for how long, the - // post-check reads the service status again after starting it. - upgradeServiceInterval = 2 * time.Second - upgradeServiceBudget = 60 * time.Second // 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 @@ -38,6 +36,24 @@ const ( // 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. @@ -93,11 +109,11 @@ func (r *upgradeRunState) verifySums() (string, int, string) { } name := filepath.Base(matches[0]) pinned := filepath.Join(r.dir, name) - if err := upgradeCopy(matches[0], pinned); err != nil { + 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)); err != nil { + 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 } @@ -135,8 +151,11 @@ func (r *upgradeRunState) verifySums() (string, int, string) { // 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. -func upgradeCopy(src, dst string) error { +// 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 @@ -146,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.