From 6aa21f5e372983d98020078ab49f07c6a904958e Mon Sep 17 00:00:00 2001 From: Said Boudjelda Date: Sun, 18 Oct 2026 16:05:54 +0200 Subject: [PATCH] fix(lab): a failed run stops the Lab's sweep, a run cancelled while starting is cancelled on the server, and diff rows keep their order Three Lab TUI faults: - A FAILED or ERROR run came back as an ordinary result. It became a 0 rec/s iteration, and a sweep, warmup or median series launched its next step against the same failure. - Pressing x while CreateTest was in flight left the run the API then started running: its labTestStartedMsg arrived with the Lab no longer running and was dropped, so nothing watched or cancelled it. - The diff's Parameter Changes rows came from ranging over a map, so they reordered on every frame. A FAILED or ERROR status now ends the run through the Lab's error path: no iteration, the series stops, and the status line names the status, with r to retry. A late labTestStartedMsg cancels its run on the server, through the cancelRun command the x key now shares. The diff rows are in key order. --- cli/tui/lab.go | 43 +++++++++++++++++--------- cli/tui/lab_test.go | 74 ++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 99 insertions(+), 18 deletions(-) diff --git a/cli/tui/lab.go b/cli/tui/lab.go index f9afdfbd..78d9b32b 100644 --- a/cli/tui/lab.go +++ b/cli/tui/lab.go @@ -4,8 +4,10 @@ import ( "context" "encoding/json" "fmt" + "maps" "os" "path/filepath" + "slices" "strings" "time" @@ -213,16 +215,7 @@ func (m LabModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // Fire the server-side cancel as a command. It used to run // synchronously HERE, inside Update — freezing the entire // TUI for up to the 30s client timeout. - var cancelCmd tea.Cmd - if id := m.runTestID; id != "" { - c := m.client - cancelCmd = func() tea.Msg { - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - _ = c.CancelTest(ctx, id) - return nil - } - } + cancelCmd := m.cancelRun(m.runTestID) m.running = false m.runTestID = "" m.sweepActive = false @@ -395,7 +388,9 @@ func (m LabModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case labTestStartedMsg: if !m.running { - return m, nil // cancelled while the test was being created + // Cancelled while the test was being created. The run the API + // started anyway used to be dropped here, and ran on unwatched. + return m, m.cancelRun(msg.run.ID) } m.runTestID = msg.run.ID m.lastTestReq = msg.req @@ -991,8 +986,9 @@ func (m LabModel) viewDiff(width int) string { paramSepW = 10 } sb.WriteString(dimStyle.Render(" "+strings.Repeat("─", paramSepW)) + "\n") - for key, aVal := range a.Params { - bVal := b.Params[key] + // In key order: ranging over the map reordered the rows every frame. + for _, key := range slices.Sorted(maps.Keys(a.Params)) { + aVal, bVal := a.Params[key], b.Params[key] if aVal != bVal { sb.WriteString(fmt.Sprintf(" %-*s %s → %s\n", colW, @@ -1044,6 +1040,20 @@ func (m LabModel) currentParams() map[string]string { return params } +// cancelRun cancels run id on the server, as a command: nil for no run. +func (m LabModel) cancelRun(id string) tea.Cmd { + if id == "" { + return nil + } + c := m.client + return func() tea.Msg { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + _ = c.CancelTest(ctx, id) + return nil + } +} + // runTest and retryTest run as tea.Cmd goroutines against a copy of the // model, so they must not assign model fields — anything the UI needs back // (run ID, request) travels in labTestStartedMsg and is applied in Update. @@ -1112,7 +1122,12 @@ func (m LabModel) pollTestOnce(testID string, deadline time.Time, maxWait time.D } status := strings.ToUpper(updated.Status) - if status == "DONE" || status == "COMPLETED" || status == "FAILED" || status == "ERROR" { + // A run that failed measured nothing. Taken as a result, it became a + // 0 rec/s iteration, and a sweep went on to its next, doomed step. + if status == "FAILED" || status == "ERROR" { + return labTestDoneMsg{testID: testID, err: fmt.Errorf("test %s ended %s", testID, status)} + } + if status == "DONE" || status == "COMPLETED" { summary, _ := m.client.ReportSummary(context.Background(), testID) return labTestDoneMsg{testID: testID, run: updated, summary: summary} } diff --git a/cli/tui/lab_test.go b/cli/tui/lab_test.go index 51385f41..ceac39b9 100644 --- a/cli/tui/lab_test.go +++ b/cli/tui/lab_test.go @@ -10,6 +10,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/bmscomp/kates/cli/client" tea "github.com/charmbracelet/bubbletea" @@ -312,8 +313,17 @@ func TestLabTestStartedMsg(t *testing.T) { } }) - t.Run("dropped when the run was cancelled during creation", func(t *testing.T) { - m := NewLab(nil, "") + // x while CreateTest was in flight dropped the run the API then started, + // and it ran on with nothing watching it. + t.Run("cancelled on the server when the run was cancelled during creation", func(t *testing.T) { + var cancels []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + cancels = append(cancels, r.Method+" "+r.URL.Path) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{}`)) + })) + t.Cleanup(srv.Close) + m := NewLab(client.New(srv.URL), "") m, cmd := updateLab(t, m, msg) if m.runTestID != "" { t.Errorf("runTestID = %q, want empty", m.runTestID) @@ -321,12 +331,68 @@ func TestLabTestStartedMsg(t *testing.T) { if m.lastTestReq != nil { t.Error("lastTestReq set from a dropped message") } - if cmd != nil { - t.Error("cmd = non-nil, want nil") + if cmd == nil { + t.Fatal("cmd = nil, want the server-side cancel of run-42") + } + cmd() + if strings.Join(cancels, ",") != "POST /api/tests/run-42/cancel" { + t.Errorf("requests = %q, want the cancel of run-42", cancels) } }) } +// A FAILED or ERROR run came back as a result: a sweep recorded a 0 rec/s +// iteration and launched its next step against the same failure. +func TestLabFailedRunStopsTheSweep(t *testing.T) { + for _, status := range []string{"FAILED", "ERROR"} { + t.Run(status, func(t *testing.T) { + t.Parallel() + m := newRunningLab(t, newLabServerClient(t, `{"id":"t1","status":"`+status+`"}`)) + m.sweepActive = true + m.sweepParam = 1 + m.params[1].Current = 0 + + done := m.pollTestOnce("t1", time.Now().Add(time.Minute), time.Minute)() + if d, ok := done.(labTestDoneMsg); !ok || d.err == nil { + t.Fatalf("poll = %#v, want a labTestDoneMsg with an error", done) + } + m, cmd := updateLab(t, m, done) + if len(m.iterations) != 0 || m.sweepActive || m.running || cmd != nil { + t.Errorf("iterations %d, sweep %v, running %v, cmd %v: want no iteration and the sweep stopped", + len(m.iterations), m.sweepActive, m.running, cmd != nil) + } + if !strings.Contains(m.status, status) { + t.Errorf("status %q does not say the run ended %s", m.status, status) + } + }) + } +} + +// The parameter rows of the diff came from ranging over a map, so they changed +// order from one frame to the next. +func TestLabDiffParameterRowsKeepTheirOrder(t *testing.T) { + m := NewLab(nil, "") + m.width, m.height = 160, 50 + m.iterations = []labIteration{ + {Number: 1, Params: map[string]string{"acks": "1", "batchSize": "16384", "compression": "lz4", "lingerMs": "0", "producers": "4"}}, + {Number: 2, Params: map[string]string{"acks": "all", "batchSize": "65536", "compression": "zstd", "lingerMs": "5", "producers": "8"}}, + } + first := m.viewDiff(160) + for range 20 { + if got := m.viewDiff(160); got != first { + t.Fatalf("the diff changed between frames:\n%s\n---\n%s", first, got) + } + } + last := -1 + for _, key := range []string{"acks", "batchSize", "compression", "lingerMs", "producers"} { + at := strings.Index(first, key) + if at < last { + t.Errorf("%s is not after the keys before it in sorted order:\n%s", key, first) + } + last = at + } +} + func TestLabTestDoneMsg(t *testing.T) { run := &client.TestRun{ ID: "t1",