Fix: Keep abctl's cached events across proxy restarts #933
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code being removed carried a specific justification this comment does not address: it existed to drop "the bootstrap
defaultbucket after rekey". That names an ID the server can plausibly recreate, rather than one it merely forgets.If
defaultcan come back as a genuinely different session, then the cache keyed on that ID now holds pre-rekey events while post-rekey events stream into the same key — so the events pane would show both under one ID with no boundary between them. Re-opening it re-snapshots, andm.events[msg.id] = trim(...)assigns rather than appends, so the mixing window is only while the user stays on the pane without returning to the picker. Narrow, and arguably even desirable as continuity.But since the new comment reframes the reconcile purely as "the server list says what is LIVE", it would be worth one sentence on whether ID reuse is possible and, if it is, that the merge is intended. As it stands the rekey rationale is deleted without a verdict.