diff --git a/authbridge/cmd/abctl/go.mod b/authbridge/cmd/abctl/go.mod index 2fe0d31ef..f1fa7de2d 100644 --- a/authbridge/cmd/abctl/go.mod +++ b/authbridge/cmd/abctl/go.mod @@ -14,6 +14,7 @@ require ( github.com/charmbracelet/bubbletea v1.3.10 github.com/charmbracelet/lipgloss v1.1.0 github.com/charmbracelet/x/ansi v0.11.8 + github.com/muesli/termenv v0.16.0 github.com/rossoctl/cortex/authbridge/authlib v0.0.0-00010101000000-000000000000 gopkg.in/yaml.v3 v3.0.1 ) @@ -46,7 +47,6 @@ require ( github.com/mattn/go-runewidth v0.0.24 // indirect github.com/muesli/ansi v0.0.0-20230316100256-276c6243b2f6 // indirect github.com/muesli/cancelreader v0.2.2 // indirect - github.com/muesli/termenv v0.16.0 // indirect github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect github.com/rivo/uniseg v0.4.7 // indirect github.com/rogpeppe/go-internal v1.15.0 // indirect diff --git a/authbridge/cmd/abctl/tui/app.go b/authbridge/cmd/abctl/tui/app.go index 6ef6ccb02..bbf699c45 100644 --- a/authbridge/cmd/abctl/tui/app.go +++ b/authbridge/cmd/abctl/tui/app.go @@ -445,6 +445,12 @@ func (m *model) backToPodsPane() { m.catalog = nil m.catalogTbl.SetRows(nil) m.previousPane = paneNone + // Close the column picker with the pane it belongs to. The paneEvents gates + // on the key block and in View() make it inert and invisible once we leave, + // but the flag itself outlives the pane: entering a session on the next pod + // puts m.pane back to paneEvents and the popup nobody reopened is there + // again, owning the keyboard until the user finds esc. + m.colPicker = false m.detailEvent = nil m.detailPlugin = nil m.selectedSess = "" @@ -609,33 +615,19 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, tickCmd() case sessionsLoadedMsg: - // Server list is authoritative. Reconcile: drop cached events for - // sessions the server no longer knows about (typically the - // bootstrap "default" bucket after rekey). If the focused session - // disappeared, back out to the sessions pane so the user isn't - // stranded on an empty events view. - serverIDs := make(map[string]bool, len(msg)) - for _, s := range msg { - serverIDs[s.ID] = true - } - for id := range m.events { - if !serverIDs[id] { - delete(m.events, id) - } - } - if m.selectedSess != "" && !serverIDs[m.selectedSess] && m.pane != paneSessions { - m.selectedSess = "" - m.pane = paneSessions - // Close the picker with the pane it belongs to. - // - // The paneEvents gates on the key block and in View() make it inert and - // invisible while the user is on the sessions table, but the flag itself - // outlived the pane: pressing enter on another session put m.pane back to - // paneEvents and the popup the user never reopened was there again, owning - // the keyboard until they found esc. Gating covers "drawn over the wrong - // pane"; this covers the return trip. - m.colPicker = false - } + // The server list says what is LIVE. It does not say what is worth + // keeping on screen. + // + // This used to drop cached events for every session the list omitted, and + // bounce the user back to the sessions pane. The session store is + // in-memory and per-pod, so abctl's copy is the only copy: a proxy restart + // (or any blip that empties /v1/sessions, which arrives as a normal + // message, not an error) destroyed the events someone was mid-investigation + // on, about two seconds after they looked away. That is #870. + // + // Cached events are now released in exactly one place: when the user + // returns to the picker and selects a different session (see keys.go). + // Nothing here deletes, and nothing here changes the focused pane. m.sessions = []session.SessionSummary(msg) m.connState.phase = connOpen m.rebuildSessionsTable() diff --git a/authbridge/cmd/abctl/tui/event_retention_test.go b/authbridge/cmd/abctl/tui/event_retention_test.go new file mode 100644 index 000000000..36b9f49fa --- /dev/null +++ b/authbridge/cmd/abctl/tui/event_retention_test.go @@ -0,0 +1,261 @@ +package tui + +import ( + "strings" + "testing" + "time" + + "github.com/charmbracelet/lipgloss" + "github.com/muesli/termenv" + + "github.com/rossoctl/cortex/authbridge/authlib/pipeline" + "github.com/rossoctl/cortex/authbridge/authlib/session" +) + +// #870: users reported the events they were investigating vanishing after a +// proxy restart or a communication blip. +// +// The store is in-memory and per-pod, so abctl's cache is the only copy. An +// empty /v1/sessions arrives as an ordinary message, not an error, so the old +// reconcile read "the server does not list this" as "delete it" and wiped the +// events about two seconds after the user looked away. +func TestSessionsRefresh_KeepsEventsAndPane(t *testing.T) { + m := newRetentionModel(t, "default", 3) + + m.Update(sessionsLoadedMsg{}) // proxy restarted: list is empty + + if got := len(m.events["default"]); got != 3 { + t.Errorf("cached events dropped on an empty server list: got %d, want 3", got) + } + if m.pane != paneEvents { + t.Errorf("pane changed under the user: got %v, want paneEvents", m.pane) + } + if m.selectedSess != "default" { + t.Errorf("selection cleared: got %q, want %q", m.selectedSess, "default") + } +} + +// The same protection while the user is on the usage charts, which are computed +// from the same cache. +func TestSessionsRefresh_KeepsUsageInvestigation(t *testing.T) { + m := newRetentionModel(t, "default", 3) + m.pane = paneUsage + + m.Update(sessionsLoadedMsg{}) + + if got := len(m.events["default"]); got != 3 { + t.Errorf("cached events dropped while on the usage pane: got %d, want 3", got) + } + if m.pane != paneUsage { + t.Errorf("pane changed under the user: got %v, want paneUsage", m.pane) + } +} + +// A session the server drops individually (evicted under max_sessions) is +// retained too — same reasoning, and the user may still be reading it. +func TestSessionsRefresh_KeepsEvictedSession(t *testing.T) { + m := newRetentionModel(t, "old", 3) + + m.Update(sessionsLoadedMsg{{ID: "fresh", UpdatedAt: time.Now()}}) + + if got := len(m.events["old"]); got != 3 { + t.Errorf("evicted session's events dropped: got %d, want 3", got) + } +} + +// Retention is only half a fix if the events cannot be reached. After a restart +// the server lists nothing, so the picker must still offer a row for whatever +// the cache holds. +func TestSessionsPicker_ListsCachedOnlySessions(t *testing.T) { + m := newRetentionModel(t, "default", 3) + + m.Update(sessionsLoadedMsg{}) + m.rebuildSessionsTable() + + var row []string + for _, r := range m.sessionsTbl.Rows() { + if r[0] == "default" { + row = r + } + } + if row == nil { + t.Fatal("no picker row for a session whose events are still cached — " + + "the retained history is unreachable") + } + if row[2] != "3" { + t.Errorf("row event count = %q, want %q", row[2], "3") + } + if row[4] != "cached" { + t.Errorf("row not marked as cached-only: %v", row) + } +} + +// The cached-only marker must survive rendering under a colour profile. bubbles +// truncates each cell with runewidth.Truncate BEFORE styling, and runewidth is +// not ANSI-aware, so a styled cell measures its escape bytes against the column +// width and comes out mangled with the reset stripped. The marker is therefore +// plain text; CI has no TTY and cannot catch a regression here, so force one. +func TestSessionsPicker_CachedMarkerRendersIntact(t *testing.T) { + orig := lipgloss.ColorProfile() + lipgloss.SetColorProfile(termenv.ANSI256) + t.Cleanup(func() { lipgloss.SetColorProfile(orig) }) + + m := newRetentionModel(t, "vanished", 3) + m.pane = paneSessions + m.Update(sessionsLoadedMsg{}) + m.rebuildSessionsTable() + + view := m.sessionsTbl.View() + if !strings.Contains(view, "cached") { + t.Errorf("marker did not survive rendering:\n%s", view) + } + if strings.Contains(view, "cache…") || strings.Contains(view, "cach…") { + t.Error("marker was truncated mid-word — a styled cell is being measured " + + "with its escape bytes counted against the column width") + } +} + +// A cache key with no events must not produce a row: snapshotLoadedMsg assigns +// m.events[id] unconditionally, so drilling into an empty session creates the +// key, and a row advertising zero events helps nobody. +func TestSessionsPicker_SkipsEmptyCacheKeys(t *testing.T) { + m := newRetentionModel(t, "real", 3) + m.events["empty"] = nil + + m.Update(sessionsLoadedMsg{}) + m.rebuildSessionsTable() + + for _, r := range m.sessionsTbl.Rows() { + if r[0] == "empty" { + t.Errorf("empty cache key produced a picker row: %v", r) + } + } +} + +// The single release point: picking a DIFFERENT session in the picker. That is +// the only reliable signal the previous events stopped mattering, and it is +// what bounds the cache now that the refresh never deletes. +func TestPickingAnotherSession_ReleasesThePrevious(t *testing.T) { + m := newRetentionModel(t, "default", 3) + m.events["other"] = make([]pipeline.SessionEvent, 2) + m.sessions = []session.SessionSummary{{ID: "default"}, {ID: "other"}} + m.rebuildSessionsTable() + + // Back to the picker, cursor on "other", press enter. + m.pane = paneSessions + for i, r := range m.sessionsTbl.Rows() { + if r[0] == "other" { + m.sessionsTbl.SetCursor(i) + } + } + m.handleKey(keyRune('l')) + + if m.selectedSess != "other" { + t.Fatalf("precondition: handler did not open \"other\" (got %q)", m.selectedSess) + } + if _, still := m.events["default"]; still { + t.Error("the previous session's events were not released") + } + if got := len(m.events["other"]); got != 2 { + t.Errorf("the newly-opened session's events were dropped: got %d, want 2", got) + } +} + +// The release loop must NOT touch cached-only sessions. Their copy is the only +// copy, so dropping one is the same unrecoverable loss as #870 — and after a +// restart every previously-visited session is cached-only, so a user with three +// such rows who opens one to read it would destroy the other two. +// +// (This test is the reason the release is scoped to live sessions rather than +// "everything except the one being opened", which is what it did first.) +func TestPickingAnotherSession_KeepsCachedOnlySessions(t *testing.T) { + m := newRetentionModel(t, "sessA", 3) + m.events["sessB"] = make([]pipeline.SessionEvent, 5) + m.events["sessC"] = make([]pipeline.SessionEvent, 7) + + // Proxy restarted: the server lists nothing, so all three are cached-only. + m.Update(sessionsLoadedMsg{}) + m.rebuildSessionsTable() + m.pane = paneSessions + for i, r := range m.sessionsTbl.Rows() { + if r[0] == "sessB" { + m.sessionsTbl.SetCursor(i) + } + } + + m.handleKey(keyRune('l')) + + for id, want := range map[string]int{"sessA": 3, "sessB": 5, "sessC": 7} { + if got := len(m.events[id]); got != want { + t.Errorf("%s: got %d events, want %d — a cached-only session was "+ + "released and its events are unrecoverable", id, got, want) + } + } +} + +// A cached-only session has no server-side counterpart, so opening one must not +// fire a snapshot: GetSession would 404 and errMsg flashes that over the very +// events this change preserves. +func TestOpeningCachedOnlySession_SkipsTheSnapshot(t *testing.T) { + m := newRetentionModel(t, "gone-session", 3) + m.Update(sessionsLoadedMsg{}) // restart; nothing is live + m.rebuildSessionsTable() + m.pane = paneSessions + + if cmd := m.handleKey(keyRune('l')); cmd != nil { + t.Error("a snapshot was issued for a session the server does not have; " + + "it will 404 and flash an error over the retained events") + } +} + +// The live case still fires one, or a session whose history has not yet streamed +// in would render empty. +func TestOpeningLiveSession_StillSnapshots(t *testing.T) { + m := newRetentionModel(t, "live", 3) + m.rebuildSessionsTable() + m.pane = paneSessions + + if cmd := m.handleKey(keyRune('l')); cmd == nil { + t.Error("no snapshot for a live session") + } +} + +// Re-opening the SAME session must not release its own events. +func TestReopeningSameSession_KeepsItsEvents(t *testing.T) { + m := newRetentionModel(t, "default", 3) + m.rebuildSessionsTable() + m.pane = paneSessions + + m.handleKey(keyRune('l')) + + if got := len(m.events["default"]); got != 3 { + t.Errorf("re-opening the same session dropped its events: got %d, want 3", got) + } +} + +func newRetentionModel(t *testing.T, id string, n int) *model { + t.Helper() + evs := make([]pipeline.SessionEvent, n) + for i := range evs { + evs[i] = pipeline.SessionEvent{ + At: time.Now(), + Direction: pipeline.Outbound, + Phase: pipeline.SessionRequest, + Host: "api.example.com", + } + } + m := &model{ + pane: paneEvents, + selectedSess: id, + width: 200, + height: 40, + bodyHeight: 12, + events: map[string][]pipeline.SessionEvent{id: evs}, + eventColumns: defaultColumnSelection(), + sessions: []session.SessionSummary{{ID: id}}, + } + m.eventsTbl = newEventsTable() + m.sessionsTbl = newSessionsTable() + m.rebuildEventsTable() + return m +} diff --git a/authbridge/cmd/abctl/tui/events_columns_test.go b/authbridge/cmd/abctl/tui/events_columns_test.go index 0c92b5048..aad2a6412 100644 --- a/authbridge/cmd/abctl/tui/events_columns_test.go +++ b/authbridge/cmd/abctl/tui/events_columns_test.go @@ -1,6 +1,7 @@ package tui import ( + "context" "strings" "testing" "time" @@ -743,12 +744,18 @@ func TestColumnPicker_DoesNotReturnAfterAnAsyncPaneChange(t *testing.T) { t.Fatal("picker did not open") } - // Drive the REAL handler, not a hand-set flag: the focused session disappears - // from the server's list, and sessionsLoadedMsg backs out to paneSessions. A - // test that assigned m.colPicker itself would pass without the fix. - m.Update(sessionsLoadedMsg{}) - if m.pane != paneSessions { - t.Fatalf("handler did not back out to paneSessions (pane=%v)", m.pane) + // Drive a REAL pane change, not a hand-set flag: a test that assigned + // m.colPicker itself would pass without the fix. + // + // This used to drive sessionsLoadedMsg with an empty list, which back then + // bounced the user to paneSessions. That bounce is deliberately gone (#870): + // a session leaving the server's list no longer moves the user or discards + // their events. The picker-lifecycle bug guarded here is independent of which + // transition triggers it, so this now drives the pane change that remains. + m.parentCtx = context.Background() + m.backToPodsPane() + if m.pane == paneEvents { + t.Fatalf("backToPodsPane left us on paneEvents (pane=%v)", m.pane) } if strings.Contains(m.View(), "COLUMNS") { t.Error("popup still drawn over the sessions pane") diff --git a/authbridge/cmd/abctl/tui/keys.go b/authbridge/cmd/abctl/tui/keys.go index c77b260d9..e1302385f 100644 --- a/authbridge/cmd/abctl/tui/keys.go +++ b/authbridge/cmd/abctl/tui/keys.go @@ -408,9 +408,47 @@ func (m *model) handleKey(msg tea.KeyMsg) tea.Cmd { if id == "" { return nil } + live := make(map[string]bool, len(m.sessions)) + for _, s := range m.sessions { + live[s.ID] = true + } + + // The one and only place cached events are released. + // + // Picking a different session in the picker is the sole reliable + // signal that the previous session's events have stopped being what + // the user is looking at. A proxy restart, or any gap in + // /v1/sessions, is not that signal — treating it as one is #870. + // + // Released only for sessions the server still LISTS: those are + // recoverable, because snapshotCmd can fetch them again. A + // cached-only session is kept — abctl's copy is the only copy, so + // dropping it would be the same unrecoverable loss this PR exists to + // stop. After a restart every previously-visited session is + // cached-only, so opening one must not destroy the rest. + // + // The honest consequence: cached-only sessions are never released + // while abctl runs, so the cache grows by one entry per restart the + // user visited a session across. Measured, that is ~165 bytes per + // event and 1000 events per session, so ~161 KB per session and a + // few MB for a long debugging afternoon — worth it, given the + // alternative is deleting the only copy of what someone is reading. + if id != m.selectedSess { + for cached := range m.events { + if cached != id && live[cached] { + delete(m.events, cached) + } + } + } m.selectedSess = id m.pane = paneEvents m.rebuildEventsTable() + if !live[id] { + // Cached-only: there is no server-side session to snapshot, so the + // fetch would 404 and flash an error over the very events this + // change preserved. + return nil + } // Snapshot in case the stream hasn't yet delivered history. return m.snapshotCmd(id) case paneEvents: diff --git a/authbridge/cmd/abctl/tui/sessions_pane.go b/authbridge/cmd/abctl/tui/sessions_pane.go index b22c28aa6..c81f1ca52 100644 --- a/authbridge/cmd/abctl/tui/sessions_pane.go +++ b/authbridge/cmd/abctl/tui/sessions_pane.go @@ -2,6 +2,7 @@ package tui import ( "fmt" + "sort" "strings" "time" @@ -53,6 +54,23 @@ func (m *model) rebuildSessionsTable() { active, }) } + // Sessions whose events abctl still holds but the server no longer lists. + // Retaining the events (#870) is only half a fix if there is no row to + // select them from: after a proxy restart the server lists nothing, so + // without this the picker is empty and the retained history is unreachable. + for _, id := range m.cachedOnlySessionIDs() { + if m.filter != "" && !strings.Contains(id, m.filter) { + continue + } + cached := m.events[id] + rows = append(rows, table.Row{ + id, + "—", + fmt.Sprintf("%d", len(cached)), + sessionTokens(0, cached), + "cached", + }) + } m.sessionsTbl.SetRows(rows) // Restore cursor position if possible. @@ -69,6 +87,26 @@ func (m *model) rebuildSessionsTable() { } } +// cachedOnlySessionIDs lists sessions abctl has events for that the server's +// current list omits, sorted so the picker does not reshuffle under the cursor +// on each refresh. Empty caches are skipped: a row advertising zero events +// helps nobody, and snapshotLoadedMsg can create the key with an empty slice. +func (m *model) cachedOnlySessionIDs() []string { + live := make(map[string]bool, len(m.sessions)) + for _, s := range m.sessions { + live[s.ID] = true + } + out := make([]string, 0, len(m.events)) + for id, evs := range m.events { + if live[id] || len(evs) == 0 { + continue + } + out = append(out, id) + } + sort.Strings(out) + return out +} + // relTime renders "Ns", "Nm", "Nh" for small deltas; absolute time otherwise. func relTime(now, t time.Time) string { if t.IsZero() {