From 676455c0f7ffa3deb8da514a865de593308e8fa0 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Wed, 23 Sep 2026 15:35:29 -0400 Subject: [PATCH 01/10] fix: Harvest titles for untitled sessions from the sessions pane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 3-minute re-harvest was gated on paneNamespaces/panePods, so an operator sitting on the sessions list — the pane a session is picked FROM — watched a new session stay a bare UUID indefinitely. Every other cell in the row refreshes on the 2s poll, so the row looked live while sessionsData stayed frozen at whatever startup loaded. Harvest when a row has no title and its session has been quiet for untitledSettleDelay. Traffic on a session means its transcript is being appended to, so keying off UpdatedAt names it seconds after the title becomes readable rather than up to reharvestInterval later. The settle delay is what keeps it affordable: the title tiers read the LAST prompt, which is the part still being written, so harvesting on the event itself would read a half-written turn and repeat every tick. Rows that already have a title trigger nothing, so the steady state costs one map lookup per row per tick. Retire the paragraph on reharvestInterval that justified the old gate — "Once a session view is up the titles on screen are already loaded" was true of a session's own events pane and false of the list it is picked from, and it is what left a new session nameless. Replaced rather than annotated, and the quoted claim is kept so the next reader can tell which reasoning is retired; this file has a history of comment paragraphs accumulating without the contradicted ones being removed. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/cmd/abctl/tui/app.go | 50 +++++++++++-- authbridge/cmd/abctl/tui/session_metadata.go | 30 ++++++++ .../cmd/abctl/tui/sessions_title_test.go | 74 +++++++++++++++++++ 3 files changed, 149 insertions(+), 5 deletions(-) diff --git a/authbridge/cmd/abctl/tui/app.go b/authbridge/cmd/abctl/tui/app.go index 8a5e39779..9b16bdd73 100644 --- a/authbridge/cmd/abctl/tui/app.go +++ b/authbridge/cmd/abctl/tui/app.go @@ -184,7 +184,7 @@ const localProbeTimeout = 2 * time.Second // stub sessions would otherwise linger in the TUI. const refreshInterval = 2 * time.Second -// reharvestInterval is how often the session picker re-reads the coding agent's transcripts. +// reharvestInterval is how often the NAMESPACES and PODS panes re-read the coding agent's transcripts. // // The picker is where someone sits while deciding which pod to open, and a session started in another // terminal meanwhile has no title until something re-reads the logs. Three minutes rather than the 2s @@ -192,10 +192,34 @@ const refreshInterval = 2 * time.Second // whose mtime has not moved — so polling it at the session cadence would re-stat the whole tree ninety // times a minute for a change that arrives every few minutes at best. // -// Only while the PICKER is open. Once a session view is up the titles on screen are already loaded, -// and a background harvest there would compete with the event stream for no visible gain. +// THIS INTERVAL IS THE NAMESPACES/PODS CADENCE ONLY, and it is no longer the only trigger. The +// sessions LIST harvests on its own terms — see untitledSettleDelay — because that pane is a +// picker too: it gains a row whenever a session appears and cannot name it without re-reading +// the transcripts. An earlier version of this paragraph said the re-harvest was "only while the +// PICKER is open" because "once a session view is up the titles on screen are already loaded", +// which was true of a session's own events pane and false of the list it is picked from. That +// reasoning is what left a new session showing a bare UUID for as long as an operator watched it. +// +// Still a fixed clock here, unlike the sessions list, and that is the remaining gap rather than a +// decision: lastHarvest is stamped when a harvest STARTS, so nothing on this path consults whether +// any transcript actually moved. The namespaces and pods panes hold no session rows to key off, +// which is why they poll instead — not because polling is the better trigger. const reharvestInterval = 3 * time.Minute +// untitledSettleDelay is how long a session must be quiet before an unnamed row triggers a +// re-harvest, and it is the whole reason this poll is affordable. +// +// New traffic means the transcript is being APPENDED TO, so harvesting the instant an event +// lands would read a file the agent is still writing — and the title tiers read the LAST +// prompt, which is exactly the part still arriving. Waiting for a pause means the read sees a +// complete turn. +// +// Five seconds because it only has to outlast the gap between events within one turn, not the +// turn itself: the harvest is incremental and re-reads only transcripts whose mtime moved, so +// being early costs a re-scan of one file rather than of the tree. Longer would make a brand +// new session sit nameless for no benefit; shorter would harvest mid-write repeatedly. +const untitledSettleDelay = 5 * time.Second + // Tea messages. type tickMsg time.Time type refreshTickMsg time.Time @@ -1113,6 +1137,22 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } return m, refreshTickCmd() } + // RE-HARVEST FOR THE SESSIONS LIST, which is a picker as much as the two panes below + // are: it gains a row whenever a session appears and cannot name it without re-reading + // the transcripts. Gated on a row that is actually unnamed AND settled, so a list whose + // titles are all known triggers nothing — see untitledSettled. + // + // STARTED HERE, NOT RETURNED FROM HERE. This pane's tick must still reach the session + // fetch at the bottom of this branch, so the harvest is batched into that return rather + // than short-circuiting it — returning early instead would trade the titles for the + // 2s refresh of every other cell in the row. + var harvestNow tea.Cmd + if m.pane == paneSessions && m.harvest != nil && !m.harvesting && + time.Since(m.lastHarvest) >= untitledSettleDelay && m.untitledSettled(time.Now()) { + m.harvesting = true + m.lastHarvest = time.Now() + harvestNow = harvestCmd(m.harvest) + } // Refresh the pipeline view too while a pane that displays plugin // Metrics is open, so counters tick rather than sitting at whatever // they were when the session was first opened. Skipped elsewhere: @@ -1123,9 +1163,9 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // requests and keep adding one every tick. if (m.pane == panePluginDetail || m.pane == panePipeline) && !m.pipelineFetching { m.pipelineFetching = true - return m, tea.Batch(m.loadSessionsCmd(), m.loadPipelineCmd(), refreshTickCmd()) + return m, tea.Batch(m.loadSessionsCmd(), m.loadPipelineCmd(), refreshTickCmd(), harvestNow) } - return m, tea.Batch(m.loadSessionsCmd(), refreshTickCmd()) + return m, tea.Batch(m.loadSessionsCmd(), refreshTickCmd(), harvestNow) case pipelineLoadedMsg: m.pipelineFetching = false diff --git a/authbridge/cmd/abctl/tui/session_metadata.go b/authbridge/cmd/abctl/tui/session_metadata.go index 02ea6f376..80b58c6f6 100644 --- a/authbridge/cmd/abctl/tui/session_metadata.go +++ b/authbridge/cmd/abctl/tui/session_metadata.go @@ -4,6 +4,7 @@ import ( "encoding/json" "io" "os" + "time" tea "github.com/charmbracelet/bubbletea" @@ -157,3 +158,32 @@ func harvestCmd(h HarvestFunc) tea.Cmd { return harvestedMsg{meta: meta} } } + +// untitledSettled reports whether some session on screen has no title yet and has been +// quiet long enough that its transcript is probably complete on disk. +// +// THE SESSIONS LIST IS A PICKER TOO, which is what this exists for. reharvestInterval was +// written for the namespaces/pods panes on the reasoning that "once a session view is +// up the titles on screen are already loaded" — true of a session's own events pane, and +// false of the list you choose a session FROM, which gains a row whenever a new session +// appears and cannot name it without re-reading the transcripts. +// +// KEYED OFF TRAFFIC, not off a wall clock. A session with events has a transcript being +// appended to, so a harvest triggered by its own updates arrives seconds after the title +// becomes readable instead of up to reharvestInterval later. The settle delay is what makes +// this cheap: without it every event on a still-unnamed session would trigger a scan, and +// with it a busy session is harvested once, after it pauses. +// +// Only sessions the metadata does NOT name are considered, so the steady state — every row +// titled — triggers nothing at all and costs one map lookup per row per tick. +func (m *model) untitledSettled(now time.Time) bool { + for _, s := range m.sessions { + if m.sessionsData[s.ID].Title != "" { + continue + } + if now.Sub(s.UpdatedAt) >= untitledSettleDelay { + return true + } + } + return false +} diff --git a/authbridge/cmd/abctl/tui/sessions_title_test.go b/authbridge/cmd/abctl/tui/sessions_title_test.go index 31e92bc12..4adb26698 100644 --- a/authbridge/cmd/abctl/tui/sessions_title_test.go +++ b/authbridge/cmd/abctl/tui/sessions_title_test.go @@ -1317,3 +1317,77 @@ func TestZeroWidthFree(t *testing.T) { } } } + +// TestSessionsPane_ReHarvestsSettledUntitledSessions pins the sessions LIST as a pane that +// re-harvests, which it was not. +// +// The re-harvest was gated on paneNamespaces/panePods, so an operator sitting on the sessions +// list — the pane they pick a session FROM — watched a new session stay a bare UUID +// indefinitely: every other cell in the row refreshes on the 2s poll, so the row looked live +// while sessionsData was frozen at whatever startup loaded. Reported from exactly that. +// +// Keyed off UpdatedAt rather than a wall clock, since traffic on a session is what says its +// transcript is being appended to. +// +// ASSERTS ON m.harvesting, NOT by running the returned batch. The sessions pane's tick also +// batches loadSessionsCmd, which dereferences a nil apiclient in a unit model — so running the +// batch panics on the fetch rather than testing the harvest. The in-flight guard is set in the +// same branch that creates the harvest command and is what the next tick reads, so it is the +// honest observable here; TestPicker_ReHarvestsOnAnInterval covers the closure actually running. +func TestSessionsPane_ReHarvestsSettledUntitledSessions(t *testing.T) { + newSessions := func(updatedAt time.Time, meta map[string]SessionMetadata) *model { + m := newTitleModel(t, meta, "s1") + m.sessions = []session.SessionSummary{{ID: "s1", UpdatedAt: updatedAt}} + m.harvest = func() (map[string]SessionMetadata, error) { + return map[string]SessionMetadata{"s1": {Title: "named at last"}}, nil + } + // Backdated so the settle delay is the only thing under test; the tick's own floor + // reuses lastHarvest and would otherwise mask it. + m.lastHarvest = time.Now().Add(-time.Hour) + return m + } + + // Settled and unnamed: the harvest starts. + m := newSessions(time.Now().Add(-2*untitledSettleDelay), map[string]SessionMetadata{}) + if _, cmd := m.Update(refreshTickMsg(time.Now())); cmd == nil { + t.Fatal("no command returned; the refresh ticker must stay armed") + } + if !m.harvesting { + t.Error("a settled untitled session did not trigger a re-harvest") + } + // And an arriving result clears the guard and reaches the table, which is the point. + m.Update(harvestedMsg{meta: map[string]SessionMetadata{"s1": {Title: "named at last"}}}) + if m.harvesting { + t.Error("harvestedMsg did not clear the in-flight guard") + } + if got := m.sessionTitle("s1"); got != "named at last" { + t.Errorf("harvested title did not reach the model: %q", got) + } + + // STILL BEING WRITTEN: an event landed just now, so the transcript's last turn may be + // mid-write and the tiers read the LAST prompt. No harvest. + m = newSessions(time.Now(), map[string]SessionMetadata{}) + m.Update(refreshTickMsg(time.Now())) + if m.harvesting { + t.Error("harvested an unsettled session, whose transcript may still be mid-write") + } + + // ALREADY NAMED: the steady state must cost nothing, or this poll would scan the + // transcript tree every two seconds forever. + m = newSessions(time.Now().Add(-2*untitledSettleDelay), + map[string]SessionMetadata{"s1": {Title: "known"}}) + m.Update(refreshTickMsg(time.Now())) + if m.harvesting { + t.Error("harvested with every row already named") + } + + // A nil harvester (--skip-claude-metadata) is never called, and the ticker stays armed. + m = newSessions(time.Now().Add(-2*untitledSettleDelay), map[string]SessionMetadata{}) + m.harvest = nil + if _, c := m.Update(refreshTickMsg(time.Now())); c == nil { + t.Error("the ticker must stay armed with no harvester") + } + if m.harvesting { + t.Error("claimed a harvest was in flight with no harvester") + } +} From a151c563b2c255feb14fcae434a4d21b64da1886 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Wed, 23 Sep 2026 16:26:59 -0400 Subject: [PATCH 02/10] fix: Back off fruitless harvests, cap the picker at one scan per visit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four review findings on the settled-untitled re-harvest. A session that can never be named kept the settle gate satisfied forever, so the sessions pane re-walked the whole transcript tree every 5s for a title that was not coming — a session whose transcript was written by a different agent, or under a CLAUDE_CONFIG_DIR that has since moved, has no title to find. The wait now doubles per fruitless harvest up to a 3m cap and resets the moment a harvest names something. Progress is "named a session this model could not name", not len(meta) > 0: an incremental harvest returns the whole merged map, so a non-empty result says nothing and would leave the backoff permanently reset. The namespaces/pods panes now harvest at most once per visit rather than every reharvestInterval. They hold no session rows, so nothing there can tell a fruitless walk from a useful one; one scan answers what the pane needs, and a session started elsewhere is named by the time an operator scrolls to it. The new-visit edge is detected by comparing pane against the previous tick's, so none of the five assignments to m.pane has to announce itself. untitledSettled now asks what the TITLE cell renders rather than reading the map: a title of " " is non-empty to Go and blank in the column, and it suppressed the harvest for a row displaying nothing. Routing the predicate through sessionTitle also means it and the cell cannot disagree about what "unnamed" means. A zero UpdatedAt is treated as unknown rather than as quiet since the epoch, which would otherwise read as settled by decades and trigger a harvest before a new session's transcript is necessarily on disk. Tests cover the previously untested lastHarvest clause, the backoff schedule including its shift-overflow range, once-per-visit, and both sanitization cases; each was mutation-checked against the code it guards. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/cmd/abctl/tui/app.go | 112 +++++++++++- authbridge/cmd/abctl/tui/session_metadata.go | 46 ++++- .../cmd/abctl/tui/sessions_title_test.go | 172 ++++++++++++++++++ 3 files changed, 323 insertions(+), 7 deletions(-) diff --git a/authbridge/cmd/abctl/tui/app.go b/authbridge/cmd/abctl/tui/app.go index 9b16bdd73..51619ad01 100644 --- a/authbridge/cmd/abctl/tui/app.go +++ b/authbridge/cmd/abctl/tui/app.go @@ -220,6 +220,41 @@ const reharvestInterval = 3 * time.Minute // new session sit nameless for no benefit; shorter would harvest mid-write repeatedly. const untitledSettleDelay = 5 * time.Second +// untitledBackoffCap bounds the exponential backoff on fruitless sessions-pane harvests. +// +// Three minutes so the worst case lands on reharvestInterval, the cadence the picker panes +// already considered acceptable for walking this tree — a session that can never be named +// then costs what the old code spent unconditionally, rather than a tree walk every +// untitledSettleDelay forever. +const untitledBackoffCap = 3 * time.Minute + +// untitledBackoff is how long to wait before the next sessions-pane harvest, given how many +// consecutive harvests have named nothing. +// +// Doubling from untitledSettleDelay: 5s, 10s, 20s … capped at untitledBackoffCap. The first +// miss still retries quickly, because the common reason a just-appeared session is unnamed is +// that its transcript was a moment behind — and that case resolves on the next attempt. +// +// SHIFTS RATHER THAN MULTIPLIES, and the shift is bounded before it is applied: misses is +// unbounded (a viewer left open overnight), and 5s << 62 overflows int64 into a negative +// duration, which would make the gate fire on EVERY tick — the exact failure the backoff +// exists to prevent, arrived at by way of the fix for it. +func untitledBackoff(misses int) time.Duration { + if misses <= 0 { + return untitledSettleDelay + } + // 2^24 * 5s is already far past the cap, so anything beyond that is the cap by definition + // and never needs to be computed. + if misses > 24 { + return untitledBackoffCap + } + d := untitledSettleDelay << uint(misses) + if d > untitledBackoffCap { + return untitledBackoffCap + } + return d +} + // Tea messages. type tickMsg time.Time type refreshTickMsg time.Time @@ -411,10 +446,32 @@ type model struct { // harvesting guards against stacking: a harvest walks a transcript tree, and a second pass while // the first is in flight would duplicate the work and race its own write of the metadata file. harvesting bool - eventCt uint64 // monotonic counter - lastTick time.Time - lastCt uint64 - rate float64 + // pickerHarvested says the namespaces/pods panes have already harvested during this visit. + // + // Cleared by the pane-change edge in the refreshTickMsg branch rather than at each site that + // assigns m.pane: there are five of those across two files, and a sixth added later would + // silently inherit "already harvested" from a previous visit. lastPane is what that edge + // compares against. + pickerHarvested bool + // lastPane is the pane the previous refresh tick saw, for detecting a pane change without + // every pane switch having to announce itself. Only the picker re-harvest reads it. + lastPane paneID + // untitledMisses counts consecutive sessions-pane harvests that named nothing, and backs the + // next one off exponentially — see untitledBackoff. + // + // A SESSION THAT CAN NEVER BE NAMED IS THE COMMON CASE HERE, not the exception: a session + // with no transcript under the agent's config dir (a different agent, a pruned tree, a + // CLAUDE_CONFIG_DIR that moved) stays unnamed no matter how often the tree is walked. Without + // a backoff its row keeps the settle gate satisfied forever, so the pane re-walks the whole + // transcript tree every untitledSettleDelay for a title that is never coming. + // + // Reset on any harvest that named something, so a tree that starts producing titles returns to + // the fast cadence immediately rather than staying penalised for earlier silence. + untitledMisses int + eventCt uint64 // monotonic counter + lastTick time.Time + lastCt uint64 + rate float64 // Connection status. connState connStateInfo @@ -1064,6 +1121,21 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case harvestedMsg: m.harvesting = false + // COUNT THE MISS BEFORE THE MERGE, since the merge is what would hide it. A harvest that + // named nothing NEW leaves every unnamed row unnamed, so the settle gate stays satisfied + // and the next tick would walk the tree again at the same cadence; untitledBackoff is what + // turns that into a widening retry. Reset on any harvest that named something, so a tree + // which starts producing titles returns to the fast cadence at once. + // + // "NAMED SOMETHING" MEANS A TITLE THIS MODEL DID NOT ALREADY HAVE, not a non-empty result. + // An incremental harvest returns the whole merged map — every session it has ever seen — + // so len(msg.meta) > 0 is true on every call once the file exists, and counting that as + // progress would leave the backoff permanently reset and the loop intact. + if m.harvestNamedSomething(msg.meta) { + m.untitledMisses = 0 + } else { + m.untitledMisses++ + } // Merge, never replace. The harvest sees one agent's config dir, while the map it is // merging into was loaded from a file that may carry entries from another dir or from a // transcript since pruned — the same reason the harvester itself upserts. Replacing @@ -1119,6 +1191,13 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, tea.Batch(m.fetchUsage(), usageTick(m.usage.tickGen)) case refreshTickMsg: + // A PANE CHANGE ENDS THE PICKER'S ONE-HARVEST-PER-VISIT BUDGET. Detected here, on the + // edge, so the five places that assign m.pane do not each have to remember to clear it — + // and so a sixth cannot quietly skip the harvest by inheriting a set flag. + if m.pane != m.lastPane { + m.lastPane = m.pane + m.pickerHarvested = false + } // In picker mode, skip the fetch — m.client may be nil after a // back-out. Keep the ticker alive so it's ready when the user // re-enters a session. @@ -1130,8 +1209,19 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // Keyed off the existing refresh ticker rather than a second tea.Tick: one timer is // easier to reason about than two with different periods, and this branch already // returns on every tick. - if m.harvest != nil && !m.harvesting && time.Since(m.lastHarvest) >= reharvestInterval { + // + // AT MOST ONCE PER VISIT TO THIS PANE, which is what pickerHarvested tracks. These + // panes hold no session rows, so there is nothing here to tell a fruitless walk from + // a useful one — the interval alone would keep re-walking the tree for as long as the + // operator sits here, and the sessions pane's own backoff cannot help because this + // path does not consult it. One walk per visit is what the pane actually needs: it + // exists so a session started elsewhere is named by the time the operator scrolls to + // it, and that is answered by a single scan. Cleared on entry to the pane, so coming + // back later harvests again. + if m.harvest != nil && !m.harvesting && !m.pickerHarvested && + time.Since(m.lastHarvest) >= reharvestInterval { m.harvesting = true + m.pickerHarvested = true m.lastHarvest = time.Now() return m, tea.Batch(harvestCmd(m.harvest), refreshTickCmd()) } @@ -1142,13 +1232,23 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // the transcripts. Gated on a row that is actually unnamed AND settled, so a list whose // titles are all known triggers nothing — see untitledSettled. // + // ONLY WHILE THIS PANE IS THE VISIBLE ONE. m.pane is exactly that — the panes above + // return before reaching here — so the check is the pane equality itself, and a harvest + // never runs on behalf of a list nobody is looking at. + // + // BACKED OFF BY untitledBackoff rather than the flat settle delay, because a session that + // can never be named keeps this gate satisfied forever: without the backoff an unnameable + // row re-walks the transcript tree every untitledSettleDelay for a title that is not + // coming. See untitledMisses. + // // STARTED HERE, NOT RETURNED FROM HERE. This pane's tick must still reach the session // fetch at the bottom of this branch, so the harvest is batched into that return rather // than short-circuiting it — returning early instead would trade the titles for the // 2s refresh of every other cell in the row. var harvestNow tea.Cmd if m.pane == paneSessions && m.harvest != nil && !m.harvesting && - time.Since(m.lastHarvest) >= untitledSettleDelay && m.untitledSettled(time.Now()) { + time.Since(m.lastHarvest) >= untitledBackoff(m.untitledMisses) && + m.untitledSettled(time.Now()) { m.harvesting = true m.lastHarvest = time.Now() harvestNow = harvestCmd(m.harvest) diff --git a/authbridge/cmd/abctl/tui/session_metadata.go b/authbridge/cmd/abctl/tui/session_metadata.go index 80b58c6f6..27bad692a 100644 --- a/authbridge/cmd/abctl/tui/session_metadata.go +++ b/authbridge/cmd/abctl/tui/session_metadata.go @@ -4,6 +4,7 @@ import ( "encoding/json" "io" "os" + "strings" "time" tea "github.com/charmbracelet/bubbletea" @@ -178,7 +179,14 @@ func harvestCmd(h HarvestFunc) tea.Cmd { // titled — triggers nothing at all and costs one map lookup per row per tick. func (m *model) untitledSettled(now time.Time) bool { for _, s := range m.sessions { - if m.sessionsData[s.ID].Title != "" { + if m.sessionHasTitle(s.ID) { + continue + } + // A ZERO UpdatedAt IS NOT "QUIET SINCE THE EPOCH". The field is whatever /v1/sessions + // sent, and a summary that omits it decodes to the zero time — which would otherwise + // read as settled by ~55 years and trigger a harvest on the first tick, before the + // transcript of a brand new session is necessarily on disk. Unknown is not settled. + if s.UpdatedAt.IsZero() { continue } if now.Sub(s.UpdatedAt) >= untitledSettleDelay { @@ -187,3 +195,39 @@ func (m *model) untitledSettled(now time.Time) bool { } return false } + +// sessionHasTitle reports whether this session renders a title, as the TITLE cell would judge it. +// +// THROUGH sessionTitle, not the raw map, so this predicate and the cell can never disagree about +// what "unnamed" means: the cell sanitises (sessionTitle does), and a predicate reading +// m.sessionsData[id].Title directly would be asserting about a different string than the one on +// screen. sanitizeLabel replaces rather than strips, so it cannot change emptiness today — the +// point is that this does not depend on that remaining true. +// +// WHITESPACE COUNTS AS UNNAMED, which the raw comparison got wrong. A title of " " is non-empty +// to Go and blank in the column, so it satisfied the old check and suppressed the harvest for a +// row displaying nothing. The harvester normalises its own output and tests each tier's CLIPPED +// value, so this is defence at the consumer rather than a live upstream bug — but this file +// renders whatever is in that map, including what an older harvester or a hand-edited file left. +func (m *model) sessionHasTitle(id string) bool { + return strings.TrimSpace(m.sessionTitle(id)) != "" +} + +// harvestNamedSomething reports whether a finished harvest named a session this model could not +// name before. +// +// NOT len(meta) > 0. An incremental harvest returns the whole merged map — every session it has +// ever seen, not just what this pass parsed — so a non-empty result says nothing about progress +// and would keep the backoff permanently reset. The question is whether any entry names a session +// that was unnamed here, which is also what the operator would call progress. +func (m *model) harvestNamedSomething(meta map[string]SessionMetadata) bool { + for id, md := range meta { + if strings.TrimSpace(sanitizeLabel(md.Title)) == "" { + continue + } + if !m.sessionHasTitle(id) { + return true + } + } + return false +} diff --git a/authbridge/cmd/abctl/tui/sessions_title_test.go b/authbridge/cmd/abctl/tui/sessions_title_test.go index 4adb26698..1bd226d57 100644 --- a/authbridge/cmd/abctl/tui/sessions_title_test.go +++ b/authbridge/cmd/abctl/tui/sessions_title_test.go @@ -1391,3 +1391,175 @@ func TestSessionsPane_ReHarvestsSettledUntitledSessions(t *testing.T) { t.Error("claimed a harvest was in flight with no harvester") } } + +// TestSessionsPane_BacksOffFruitlessHarvests pins the exponential backoff. +// +// A session with no transcript under the agent's config dir can never be named — a different +// agent wrote it, the tree was pruned, CLAUDE_CONFIG_DIR moved. Its row keeps the settle gate +// satisfied forever, so without a backoff the pane re-walks the whole transcript tree every +// untitledSettleDelay for a title that is not coming. +func TestSessionsPane_BacksOffFruitlessHarvests(t *testing.T) { + m := newTitleModel(t, map[string]SessionMetadata{}, "s1") + m.sessions = []session.SessionSummary{{ID: "s1", UpdatedAt: time.Now().Add(-time.Hour)}} + m.harvest = func() (map[string]SessionMetadata, error) { return nil, nil } + + // A harvest that names nothing widens the wait. + for want := 1; want <= 3; want++ { + m.lastHarvest = time.Now().Add(-untitledBackoffCap) + m.Update(refreshTickMsg(time.Now())) + if !m.harvesting { + t.Fatalf("miss %d: no harvest started with the backoff elapsed", want) + } + m.Update(harvestedMsg{}) + if m.untitledMisses != want { + t.Fatalf("after %d fruitless harvests untitledMisses = %d", want, m.untitledMisses) + } + } + + // THE WIDENED WAIT IS ACTUALLY ENFORCED. Backdated by the PREVIOUS step's delay, which the + // flat settle delay would have accepted; the current backoff must not. + m.lastHarvest = time.Now().Add(-untitledBackoff(m.untitledMisses - 1)) + m.Update(refreshTickMsg(time.Now())) + if m.harvesting { + t.Error("harvested before the backed-off interval had elapsed") + } + + // A harvest that names something resets to the fast cadence. + m.lastHarvest = time.Now().Add(-untitledBackoffCap) + m.Update(refreshTickMsg(time.Now())) + if !m.harvesting { + t.Fatal("no harvest started with the backoff fully elapsed") + } + m.Update(harvestedMsg{meta: map[string]SessionMetadata{"s1": {Title: "named at last"}}}) + if m.untitledMisses != 0 { + t.Errorf("a harvest that named a session left untitledMisses = %d", m.untitledMisses) + } +} + +// TestUntitledBackoff_DoublesAndIsBounded pins the schedule, including the overflow guard. +// +// misses is unbounded — a viewer left open overnight keeps counting — and an unguarded +// `untitledSettleDelay << misses` goes NEGATIVE past 62, which would make the gate fire on every +// tick: the exact failure the backoff exists to prevent, reached by way of its own fix. +func TestUntitledBackoff_DoublesAndIsBounded(t *testing.T) { + if got := untitledBackoff(0); got != untitledSettleDelay { + t.Errorf("untitledBackoff(0) = %v, want the flat settle delay %v", got, untitledSettleDelay) + } + if got := untitledBackoff(1); got != 2*untitledSettleDelay { + t.Errorf("untitledBackoff(1) = %v, want %v", got, 2*untitledSettleDelay) + } + // Monotonic, never negative, never past the cap — including the shift-overflow range. + prev := time.Duration(0) + for _, misses := range []int{0, 1, 2, 3, 8, 24, 25, 62, 63, 64, 1 << 20} { + got := untitledBackoff(misses) + if got <= 0 { + t.Fatalf("untitledBackoff(%d) = %v, must be positive", misses, got) + } + if got > untitledBackoffCap { + t.Errorf("untitledBackoff(%d) = %v, past the cap %v", misses, got, untitledBackoffCap) + } + if got < prev { + t.Errorf("untitledBackoff(%d) = %v went backwards from %v", misses, got, prev) + } + prev = got + } +} + +// TestPicker_HarvestsAtMostOncePerVisit pins the picker to one tree walk per visit. +// +// The namespaces/pods panes hold no session rows, so nothing there can tell a fruitless walk from +// a useful one and the interval alone would re-walk the tree for as long as the operator sits +// there. One scan answers what the pane needs: a session started elsewhere is named by the time +// they scroll to it. +func TestPicker_HarvestsAtMostOncePerVisit(t *testing.T) { + m := newTitleModel(t, map[string]SessionMetadata{}, "s1") + m.pane = paneNamespaces + m.lastPane = paneNamespaces + called := 0 + m.harvest = func() (map[string]SessionMetadata, error) { + called++ + return nil, nil + } + + m.lastHarvest = time.Now().Add(-2 * reharvestInterval) + _, cmd := m.Update(refreshTickMsg(time.Now())) + runBatch(t, cmd) + if called != 1 { + t.Fatalf("first tick harvested %d times, want 1", called) + } + m.Update(harvestedMsg{}) + + // A second due tick in the same visit must not walk the tree again. + m.lastHarvest = time.Now().Add(-2 * reharvestInterval) + _, again := m.Update(refreshTickMsg(time.Now())) + runBatch(t, again) + if called != 1 { + t.Errorf("a second tick in the same visit harvested again (%d calls)", called) + } + + // Leaving and returning is a new visit, detected on the pane-change edge rather than by + // every assignment to m.pane announcing itself. + m.pane = paneSessions + m.Update(refreshTickMsg(time.Now())) + m.pane = paneNamespaces + m.lastHarvest = time.Now().Add(-2 * reharvestInterval) + _, revisit := m.Update(refreshTickMsg(time.Now())) + runBatch(t, revisit) + if called != 2 { + t.Errorf("a return to the picker did not harvest again (%d calls, want 2)", called) + } +} + +// TestUntitledSettled_BlankAndUnknownRows pins what counts as unnamed and as settled. +// +// A title of " " is non-empty to Go and blank in the column, so a raw `!= ""` suppressed the +// harvest for a row displaying nothing. A zero UpdatedAt is unknown, not quiet since the epoch. +func TestUntitledSettled_BlankAndUnknownRows(t *testing.T) { + settled := time.Now().Add(-2 * untitledSettleDelay) + + cases := []struct { + name string + meta map[string]SessionMetadata + upd time.Time + want bool + }{ + {"unnamed and settled", map[string]SessionMetadata{}, settled, true}, + {"named", map[string]SessionMetadata{"s1": {Title: "known"}}, settled, false}, + {"whitespace title is unnamed", map[string]SessionMetadata{"s1": {Title: " "}}, settled, true}, + // A CONTROL CHARACTER IS NAMED, not blank. sanitizeLabel REPLACES it with U+FFFD rather + // than stripping it, so the cell shows a visible glyph and TrimSpace does not remove it. + // Pinned to record which side of the line this falls on: the predicate asks what the cell + // renders, and the cell renders something here. + {"control-only title renders a glyph", map[string]SessionMetadata{"s1": {Title: "\t"}}, settled, false}, + {"unsettled", map[string]SessionMetadata{}, time.Now(), false}, + {"unknown UpdatedAt is not settled", map[string]SessionMetadata{}, time.Time{}, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + m := newTitleModel(t, tc.meta, "s1") + m.sessions = []session.SessionSummary{{ID: "s1", UpdatedAt: tc.upd}} + if got := m.untitledSettled(time.Now()); got != tc.want { + t.Errorf("untitledSettled = %v, want %v", got, tc.want) + } + }) + } +} + +// TestHarvestNamedSomething_IgnoresAlreadyKnown pins the backoff's notion of progress. +// +// An incremental harvest returns the whole MERGED map — every session it has ever seen, not just +// what this pass parsed — so len(meta) > 0 is true on every call once the file exists. Counting +// that as progress would leave the backoff permanently reset and the retry loop intact. +func TestHarvestNamedSomething_IgnoresAlreadyKnown(t *testing.T) { + m := newTitleModel(t, map[string]SessionMetadata{"s1": {Title: "known"}}, "s1") + + if m.harvestNamedSomething(map[string]SessionMetadata{"s1": {Title: "known"}}) { + t.Error("a map repeating a title this model already had counted as progress") + } + if m.harvestNamedSomething(map[string]SessionMetadata{"s2": {Title: " "}}) { + t.Error("a blank title counted as naming a session") + } + if !m.harvestNamedSomething(map[string]SessionMetadata{"s2": {Title: "new name"}}) { + t.Error("a title for a session this model could not name was not counted") + } +} From 1e598ee32fcc052c7cd39cfdc59c250fd83dc870 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Wed, 23 Sep 2026 16:55:49 -0400 Subject: [PATCH 03/10] fix: Harvest the picker on arrival and score progress from the screen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three corrections to a151c563, all on the backoff's accounting. The namespaces/pods panes now harvest exactly once per visit, on arrival, with no interval. Harvesting there is opportunistic — the picker is rarely visited, and the only reason to walk a transcript tree from it is that the machine is otherwise idle waiting for someone to choose a pod — so the scan belongs at the moment you arrive rather than paced by a clock some earlier harvest set. Once pickerHarvested capped the pane at one scan per visit, the 3-minute interval could only suppress that scan: entering the picker less than three minutes after any other harvest skipped the visit's only walk, with nothing left to retry it until the operator left and came back. reharvestInterval had no other use and is retired. harvestNamedSomething now iterates the sessions on screen instead of the harvest result. An incremental harvest returns the whole merged map — ~180 entries on a developer laptop against the handful a pod serves — so walking the result and asking "is this id unnamed here?" answered yes on the first historical session the model had no metadata for, on every call. That pinned untitledMisses at zero and defeated the backoff just as thoroughly as len(meta) > 0 would have, which the previous commit's comment claimed to avoid. Asked from the screen inward it means what it says: does this result name a row the viewer is showing and could not name? A harvest that arrives with no session rows is no longer scored at all. The predicate compares against m.sessions, so with that list empty it returns false however much the harvest learned, and counting it moved the backoff on no evidence. That is the ordinary path rather than a corner: Init harvests before the session fetch batched alongside it returns, backing out to the picker sets m.sessions to nil, and the picker now harvests on arrival — so the normal route into a session list inflated untitledMisses several steps before the first row was drawn. Unscoreable is neither progress nor a miss, so the counter holds. TestPicker_ReHarvestsOnAnInterval becomes TestPicker_HarvestsOnArrival, keeping its in-flight stacking assertions and inverting the interval one. Each new guard was mutation-checked against the code it protects. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/cmd/abctl/tui/app.go | 85 +++++++----- authbridge/cmd/abctl/tui/session_metadata.go | 38 +++--- .../cmd/abctl/tui/sessions_title_test.go | 126 ++++++++++++------ 3 files changed, 158 insertions(+), 91 deletions(-) diff --git a/authbridge/cmd/abctl/tui/app.go b/authbridge/cmd/abctl/tui/app.go index 51619ad01..6ba3cf25b 100644 --- a/authbridge/cmd/abctl/tui/app.go +++ b/authbridge/cmd/abctl/tui/app.go @@ -184,27 +184,30 @@ const localProbeTimeout = 2 * time.Second // stub sessions would otherwise linger in the TUI. const refreshInterval = 2 * time.Second -// reharvestInterval is how often the NAMESPACES and PODS panes re-read the coding agent's transcripts. +// pickerHarvestOnce records that the NAMESPACES and PODS panes harvest ONCE PER VISIT, on +// arrival, with no interval at all. There is no constant here because there is no longer a +// cadence to name — the visit itself is the trigger. // -// The picker is where someone sits while deciding which pod to open, and a session started in another -// terminal meanwhile has no title until something re-reads the logs. Three minutes rather than the 2s -// refresh tick: a harvest walks a transcript tree, and the incremental pass only skips work for files -// whose mtime has not moved — so polling it at the session cadence would re-stat the whole tree ninety -// times a minute for a change that arrives every few minutes at best. +// OPPORTUNISTIC, NOT NEEDED BY THE PANE. This is the reasoning that removed the interval, and +// it is not the reasoning the interval was written under. The picker is rarely visited, and the +// only reason to walk a transcript tree from it is that the system is otherwise idle waiting for +// someone to choose a pod — so the scan is work taken while nothing else wants the machine, not +// work the pane depends on. Idle-time work belongs at the moment you arrive; pacing it on a +// clock set by some earlier harvest gets the timing exactly backwards. // -// THIS INTERVAL IS THE NAMESPACES/PODS CADENCE ONLY, and it is no longer the only trigger. The -// sessions LIST harvests on its own terms — see untitledSettleDelay — because that pane is a -// picker too: it gains a row whenever a session appears and cannot name it without re-reading -// the transcripts. An earlier version of this paragraph said the re-harvest was "only while the -// PICKER is open" because "once a session view is up the titles on screen are already loaded", -// which was true of a session's own events pane and false of the list it is picked from. That -// reasoning is what left a new session showing a bare UUID for as long as an operator watched it. +// A 3-MINUTE INTERVAL USED TO GUARD THIS, and once pickerHarvested existed it did nothing but +// SUPPRESS the single scan each visit was allowed: entering the picker less than three minutes +// after any other harvest skipped the visit's only walk, with nothing left to retry it until the +// operator left and came back after the clock aged out. An earlier version of this paragraph +// justified the interval by saying a harvest is expensive enough that polling it at the 2s +// session cadence would re-stat the whole tree ninety times a minute — true, and the reason the +// interval was right when it was the only thing limiting repeats. pickerHarvested limits them +// now, so the clock only ever cost a scan. // -// Still a fixed clock here, unlike the sessions list, and that is the remaining gap rather than a -// decision: lastHarvest is stamped when a harvest STARTS, so nothing on this path consults whether -// any transcript actually moved. The namespaces and pods panes hold no session rows to key off, -// which is why they poll instead — not because polling is the better trigger. -const reharvestInterval = 3 * time.Minute +// The sessions LIST harvests on entirely separate terms — see untitledSettleDelay and +// untitledBackoff — because it gains a row whenever a session appears and can tell an unnamed +// row from a named one. These panes hold no session rows at all, which is why one scan per visit +// is the most they can sensibly ask for. // untitledSettleDelay is how long a session must be quiet before an unnamed row triggers a // re-harvest, and it is the whole reason this poll is affordable. @@ -222,10 +225,10 @@ const untitledSettleDelay = 5 * time.Second // untitledBackoffCap bounds the exponential backoff on fruitless sessions-pane harvests. // -// Three minutes so the worst case lands on reharvestInterval, the cadence the picker panes -// already considered acceptable for walking this tree — a session that can never be named -// then costs what the old code spent unconditionally, rather than a tree walk every -// untitledSettleDelay forever. +// Three minutes because that is what the picker panes used to spend on this tree +// unconditionally, back when a fixed interval paced them — a cadence nobody objected to for +// walking a transcript tree. A session that can never be named now costs that at worst, rather +// than a tree walk every untitledSettleDelay forever. const untitledBackoffCap = 3 * time.Minute // untitledBackoff is how long to wait before the next sessions-pane harvest, given how many @@ -1131,10 +1134,24 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // An incremental harvest returns the whole merged map — every session it has ever seen — // so len(msg.meta) > 0 is true on every call once the file exists, and counting that as // progress would leave the backoff permanently reset and the loop intact. - if m.harvestNamedSomething(msg.meta) { - m.untitledMisses = 0 - } else { - m.untitledMisses++ + // SCORED ONLY WHEN THERE ARE ROWS TO SCORE IT AGAINST. harvestNamedSomething asks whether + // this result names a session m.sessions holds and could not name, so with that list empty + // the answer is false no matter how much the harvest learned. Counting it would move the + // backoff on no evidence, and it is the common case rather than a corner: Init harvests + // before the session fetch it is batched alongside has returned, and backing out to the + // picker sets m.sessions to nil, so every picker-mode harvest lands with zero rows. The + // picker now harvests on arrival, which is the normal way in — so without this guard the + // ordinary path to a session list would inflate untitledMisses several steps before the + // first row was ever drawn, and the backoff would start already widened. + // + // UNSCOREABLE IS NEITHER, so the counter holds rather than resetting: a harvest nobody + // could judge is no reason to believe the tree started producing titles either. + if len(m.sessions) > 0 { + if m.harvestNamedSomething(msg.meta) { + m.untitledMisses = 0 + } else { + m.untitledMisses++ + } } // Merge, never replace. The harvest sees one agent's config dir, while the map it is // merging into was loaded from a file that may carry entries from another dir or from a @@ -1210,16 +1227,12 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // easier to reason about than two with different periods, and this branch already // returns on every tick. // - // AT MOST ONCE PER VISIT TO THIS PANE, which is what pickerHarvested tracks. These - // panes hold no session rows, so there is nothing here to tell a fruitless walk from - // a useful one — the interval alone would keep re-walking the tree for as long as the - // operator sits here, and the sessions pane's own backoff cannot help because this - // path does not consult it. One walk per visit is what the pane actually needs: it - // exists so a session started elsewhere is named by the time the operator scrolls to - // it, and that is answered by a single scan. Cleared on entry to the pane, so coming - // back later harvests again. - if m.harvest != nil && !m.harvesting && !m.pickerHarvested && - time.Since(m.lastHarvest) >= reharvestInterval { + // EXACTLY ONCE PER VISIT, ON ARRIVAL, which is what pickerHarvested tracks — there is + // no interval here on purpose; see pickerHarvestOnce. These panes hold no session rows, + // so nothing here can tell a fruitless walk from a useful one, and the sessions pane's + // backoff cannot help because this path does not consult it. Cleared on entry to the + // pane, so coming back later harvests again. + if m.harvest != nil && !m.harvesting && !m.pickerHarvested { m.harvesting = true m.pickerHarvested = true m.lastHarvest = time.Now() diff --git a/authbridge/cmd/abctl/tui/session_metadata.go b/authbridge/cmd/abctl/tui/session_metadata.go index 27bad692a..0bf76d433 100644 --- a/authbridge/cmd/abctl/tui/session_metadata.go +++ b/authbridge/cmd/abctl/tui/session_metadata.go @@ -163,15 +163,15 @@ func harvestCmd(h HarvestFunc) tea.Cmd { // untitledSettled reports whether some session on screen has no title yet and has been // quiet long enough that its transcript is probably complete on disk. // -// THE SESSIONS LIST IS A PICKER TOO, which is what this exists for. reharvestInterval was -// written for the namespaces/pods panes on the reasoning that "once a session view is -// up the titles on screen are already loaded" — true of a session's own events pane, and -// false of the list you choose a session FROM, which gains a row whenever a new session -// appears and cannot name it without re-reading the transcripts. +// THE SESSIONS LIST IS A PICKER TOO, which is what this exists for. The namespaces/pods +// re-harvest was written on the reasoning that "once a session view is up the titles on +// screen are already loaded" — true of a session's own events pane, and false of the list +// you choose a session FROM, which gains a row whenever a new session appears and cannot +// name it without re-reading the transcripts. // // KEYED OFF TRAFFIC, not off a wall clock. A session with events has a transcript being // appended to, so a harvest triggered by its own updates arrives seconds after the title -// becomes readable instead of up to reharvestInterval later. The settle delay is what makes +// becomes readable instead of minutes later. The settle delay is what makes // this cheap: without it every event on a still-unnamed session would trigger a scan, and // with it a busy session is harvested once, after it pauses. // @@ -213,19 +213,25 @@ func (m *model) sessionHasTitle(id string) bool { return strings.TrimSpace(m.sessionTitle(id)) != "" } -// harvestNamedSomething reports whether a finished harvest named a session this model could not -// name before. -// -// NOT len(meta) > 0. An incremental harvest returns the whole merged map — every session it has -// ever seen, not just what this pass parsed — so a non-empty result says nothing about progress -// and would keep the backoff permanently reset. The question is whether any entry names a session -// that was unnamed here, which is also what the operator would call progress. +// harvestNamedSomething reports whether a finished harvest named a session that is ON SCREEN and +// was unnamed. +// +// ITERATES m.sessions, NOT THE RESULT MAP, and that distinction is the whole function. An +// incremental harvest returns the whole merged map — every session it has ever seen, roughly 180 +// entries on a developer laptop against the handful a pod is currently serving. Walking the result +// and asking "is this id unnamed here?" therefore answers yes on the first historical session the +// model has no metadata for, on every single call, which pins the backoff at zero and defeats it +// just as surely as len(meta) > 0 would. An earlier version did exactly that. +// +// So the question is asked from the screen inward: for each row the viewer is showing and cannot +// name, does this result name it? That is also what an operator would call progress — a title +// arriving for a session nobody is looking at is not why the backoff exists. func (m *model) harvestNamedSomething(meta map[string]SessionMetadata) bool { - for id, md := range meta { - if strings.TrimSpace(sanitizeLabel(md.Title)) == "" { + for _, sess := range m.sessions { + if m.sessionHasTitle(sess.ID) { continue } - if !m.sessionHasTitle(id) { + if strings.TrimSpace(sanitizeLabel(meta[sess.ID].Title)) != "" { return true } } diff --git a/authbridge/cmd/abctl/tui/sessions_title_test.go b/authbridge/cmd/abctl/tui/sessions_title_test.go index 1bd226d57..75040757c 100644 --- a/authbridge/cmd/abctl/tui/sessions_title_test.go +++ b/authbridge/cmd/abctl/tui/sessions_title_test.go @@ -1082,13 +1082,18 @@ func runBatch(t *testing.T, cmd tea.Cmd) { } } -// The session picker re-harvests periodically, so a session started elsewhere gets named. +// The session picker harvests on arrival, so a session started elsewhere gets named. // -// The picker is the one pane where someone may sit for minutes with nothing refreshing the titles: -// the 2s tick skips the session fetch there (m.client may be nil), and the harvest previously ran -// once at Init. A session started in another terminal meanwhile stayed nameless until the viewer was -// restarted. -func TestPicker_ReHarvestsOnAnInterval(t *testing.T) { +// The picker is the one pane where someone may sit with nothing refreshing the titles: the 2s tick +// skips the session fetch there (m.client may be nil), and the harvest once ran only at Init, so a +// session started in another terminal stayed nameless until the viewer was restarted. +// +// ON ARRIVAL, NOT ON AN INTERVAL, which is what this test was renamed from. The interval it used to +// assert has been removed: once pickerHarvested capped the pane at one scan per visit, the clock +// could only SUPPRESS that scan — entering the picker soon after any other harvest skipped the +// visit's only walk. What remains worth pinning here is the in-flight stacking guard, which is why +// that part of the test is unchanged. +func TestPicker_HarvestsOnArrival(t *testing.T) { newPicker := func(harvest HarvestFunc) *model { m := newTitleModel(t, map[string]SessionMetadata{}) m.pane = paneNamespaces @@ -1101,38 +1106,28 @@ func TestPicker_ReHarvestsOnAnInterval(t *testing.T) { return map[string]SessionMetadata{"s1": {Title: "found later"}}, nil } - // Not yet due: the stamp is fresh, so the tick only re-arms the timer. + // A FRESH STAMP MUST NOT SUPPRESS THE SCAN. This is the removed interval, stated as the + // assertion it now fails: lastHarvest is seconds old — as it is on arriving in the picker just + // after startup — and the visit's harvest must still run. m := newPicker(harvest) m.lastHarvest = time.Now() - if _, cmd := m.Update(refreshTickMsg(time.Now())); cmd == nil { - t.Fatal("the refresh ticker was not re-armed") - } - if called != 0 { - t.Errorf("harvested %d times while not due, want 0", called) - } - - // Due: the returned command must include the harvest, which we run to observe it. - m = newPicker(harvest) - m.lastHarvest = time.Now().Add(-2 * reharvestInterval) _, cmd := m.Update(refreshTickMsg(time.Now())) if cmd == nil { - t.Fatal("no command returned when a re-harvest was due") + t.Fatal("the refresh ticker was not re-armed") } - // tea.Batch returns a BatchMsg holding the member commands rather than running them, so the - // members have to be invoked to reach the harvest closure. runBatch(t, cmd) - if called == 0 { - t.Error("a due re-harvest did not run the harvester") + if called != 1 { + t.Fatalf("harvested %d times on arrival with a fresh stamp, want 1", called) } if !m.harvesting { t.Error("the in-flight guard was not set, so a second tick could stack a harvest") } - // While one is in flight, a further tick must not start another — even once the interval has - // elapsed again. Backdating the stamp is what makes this test the guard's: without it the tick is - // simply not due, so the assertion passed with the guard removed (confirmed by mutation). + // While one is in flight, a further tick must not start another. Clearing pickerHarvested is + // what makes this test the GUARD's: leaving it set would stop the second tick by itself, so the + // assertion would pass with the in-flight check removed. before := called - m.lastHarvest = time.Now().Add(-2 * reharvestInterval) + m.pickerHarvested = false _, stacked := m.Update(refreshTickMsg(time.Now())) runBatch(t, stacked) if called != before { @@ -1150,7 +1145,6 @@ func TestPicker_ReHarvestsOnAnInterval(t *testing.T) { // A nil harvester (--skip-claude-metadata) must never be called. m = newPicker(nil) - m.lastHarvest = time.Now().Add(-2 * reharvestInterval) if _, c := m.Update(refreshTickMsg(time.Now())); c == nil { t.Error("the ticker must stay armed even with no harvester") } @@ -1481,7 +1475,6 @@ func TestPicker_HarvestsAtMostOncePerVisit(t *testing.T) { return nil, nil } - m.lastHarvest = time.Now().Add(-2 * reharvestInterval) _, cmd := m.Update(refreshTickMsg(time.Now())) runBatch(t, cmd) if called != 1 { @@ -1489,8 +1482,7 @@ func TestPicker_HarvestsAtMostOncePerVisit(t *testing.T) { } m.Update(harvestedMsg{}) - // A second due tick in the same visit must not walk the tree again. - m.lastHarvest = time.Now().Add(-2 * reharvestInterval) + // A second tick in the same visit must not walk the tree again. _, again := m.Update(refreshTickMsg(time.Now())) runBatch(t, again) if called != 1 { @@ -1502,7 +1494,6 @@ func TestPicker_HarvestsAtMostOncePerVisit(t *testing.T) { m.pane = paneSessions m.Update(refreshTickMsg(time.Now())) m.pane = paneNamespaces - m.lastHarvest = time.Now().Add(-2 * reharvestInterval) _, revisit := m.Update(refreshTickMsg(time.Now())) runBatch(t, revisit) if called != 2 { @@ -1545,21 +1536,78 @@ func TestUntitledSettled_BlankAndUnknownRows(t *testing.T) { } } -// TestHarvestNamedSomething_IgnoresAlreadyKnown pins the backoff's notion of progress. +// TestHarvestNamedSomething_JudgesOnlyRowsOnScreen pins the backoff's notion of progress. // -// An incremental harvest returns the whole MERGED map — every session it has ever seen, not just -// what this pass parsed — so len(meta) > 0 is true on every call once the file exists. Counting -// that as progress would leave the backoff permanently reset and the retry loop intact. -func TestHarvestNamedSomething_IgnoresAlreadyKnown(t *testing.T) { +// Two ways to get this wrong, and the function had the second one. len(meta) > 0 is true on every +// call once the file exists, because an incremental harvest returns the whole MERGED map. Walking +// that map and asking "is this id unnamed here?" fails the same way for a subtler reason: the map +// carries every session the harvester has ever seen — ~180 on a laptop against the few a pod serves +// — so the first historical id the model has no metadata for answers yes, every time, pinning the +// backoff at zero. +func TestHarvestNamedSomething_JudgesOnlyRowsOnScreen(t *testing.T) { m := newTitleModel(t, map[string]SessionMetadata{"s1": {Title: "known"}}, "s1") if m.harvestNamedSomething(map[string]SessionMetadata{"s1": {Title: "known"}}) { t.Error("a map repeating a title this model already had counted as progress") } - if m.harvestNamedSomething(map[string]SessionMetadata{"s2": {Title: " "}}) { + // THE HISTORICAL-SESSIONS CASE. "old" is not a row on screen, so naming it is not progress + // toward naming what the viewer is showing — this is the assertion that fails if the function + // goes back to iterating the result map. + if m.harvestNamedSomething(map[string]SessionMetadata{"old": {Title: "some session from last week"}}) { + t.Error("a title for a session that is not on screen counted as progress") + } + + // An unnamed row on screen, which the harvest can make progress on. + m = newTitleModel(t, map[string]SessionMetadata{}, "s1") + if m.harvestNamedSomething(map[string]SessionMetadata{"s1": {Title: " "}}) { t.Error("a blank title counted as naming a session") } - if !m.harvestNamedSomething(map[string]SessionMetadata{"s2": {Title: "new name"}}) { - t.Error("a title for a session this model could not name was not counted") + if m.harvestNamedSomething(map[string]SessionMetadata{"old": {Title: "elsewhere"}}) { + t.Error("a title for the wrong session counted as naming the row on screen") + } + if !m.harvestNamedSomething(map[string]SessionMetadata{"s1": {Title: "new name"}}) { + t.Error("a title for the unnamed row on screen was not counted") + } +} + +// TestHarvestedMsg_UnscoreableHarvestsDoNotMoveTheBackoff pins fix A. +// +// harvestNamedSomething asks whether a result names a session m.sessions holds and could not name, +// so with that list EMPTY the answer is false however much the harvest learned. Counting it moved +// the backoff on no evidence — and it is the ordinary path, not a corner: Init harvests before the +// session fetch batched alongside it returns, backing out to the picker sets m.sessions to nil, and +// the picker harvests on arrival. So the normal route into a session list used to inflate +// untitledMisses several steps before the first row was drawn, starting the backoff already widened. +func TestHarvestedMsg_UnscoreableHarvestsDoNotMoveTheBackoff(t *testing.T) { + // A harvest arriving with no session rows: neither progress nor a miss. + m := newTitleModel(t, map[string]SessionMetadata{}, "s1") + m.sessions = nil + for i := 0; i < 3; i++ { + m.Update(harvestedMsg{meta: map[string]SessionMetadata{"s1": {Title: "learned plenty"}}}) + } + if m.untitledMisses != 0 { + t.Errorf("harvests with no rows to judge moved the counter to %d, want 0", m.untitledMisses) + } + + // HOLDS, rather than resetting. A harvest nobody could judge is no evidence the tree started + // producing titles either, so an already-widened backoff must not be cleared by one. + m.untitledMisses = 4 + m.sessions = nil + m.Update(harvestedMsg{meta: map[string]SessionMetadata{"s1": {Title: "learned plenty"}}}) + if m.untitledMisses != 4 { + t.Errorf("an unscoreable harvest reset the counter to %d, want it held at 4", m.untitledMisses) + } + + // With rows present the scoring is unchanged — the guard narrows when counting happens, not + // what counting means. + m = newTitleModel(t, map[string]SessionMetadata{}, "s1") + m.sessions = []session.SessionSummary{{ID: "s1", UpdatedAt: time.Now()}} + m.Update(harvestedMsg{meta: map[string]SessionMetadata{"other": {Title: "not on screen"}}}) + if m.untitledMisses != 1 { + t.Errorf("a fruitless harvest with rows present left untitledMisses = %d, want 1", m.untitledMisses) + } + m.Update(harvestedMsg{meta: map[string]SessionMetadata{"s1": {Title: "named"}}}) + if m.untitledMisses != 0 { + t.Errorf("a harvest that named a visible row left untitledMisses = %d, want 0", m.untitledMisses) } } From cba635d9582b6d8e45a9d861c471527344f13a2b Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Wed, 23 Sep 2026 20:37:29 -0400 Subject: [PATCH 04/10] fix: Make the picker's harvest budget span the visit, not the pane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five review findings on the harvest gating, three of them defects in the two commits before this one. The one-harvest-per-visit budget was keyed on the exact pane, but a visit spans two: enter on a namespace goes to panePods and esc comes back. Each hop tripped the pane-change edge and cleared the flag, so drilling into a namespace and backing out re-walked the whole transcript tree per keystroke — the opposite of what the cap is for. The edge now compares whether the picker is showing at all. That also retires the zero-value trap the reviewer flagged separately. lastPane was a paneID, and paneNamespaces is iota 0, so a picker model — which starts on paneNamespaces — recorded "already here" before any tick ran and never saw its first arrival. It harvested anyway only because Init scans separately. Replacing the field with a pickerShowing bool makes false the honest zero (no tick has seen the picker yet), so both constructors are correct without seeding anything, rather than needing the paneNone sentinel previousPane and pipelineReturnPane each carry. backToPodsPane cleared ~20 fields describing the pod being left but not untitledMisses, so a widened backoff priced the NEXT pod's first harvest: at the cap, 3 minutes before its first scan instead of the 5s settle delay. Verified before fixing. Also: titleIsBlank extracted, because harvestNamedSomething had an inline copy of the unnamed predicate that sessionHasTitle's own comment argues exists to prevent exactly that drift — and it is the one caller passing a RAW harvest result, where sessionTitle has not already sanitised; a single now hoisted so the sessions gate's backoff and settle tests are answered about one instant; the stale "five of those across two files" count corrected to two dozen; and the harvesting invariant documented at the sole site that clears it, since nothing else would catch a future path that drops harvestedMsg and latches the flag forever. Tests: the namespaces->pods and pods->namespaces hops not re-harvesting, the first tick harvesting from a constructor-shaped model, the back-out reset, and a control-only title on the raw-input path. Five mutations run against the new guards, all caught — including one that showed the sanitize in titleIsBlank was unguarded until that last case was added. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/cmd/abctl/tui/app.go | 65 ++++++++++--- authbridge/cmd/abctl/tui/session_metadata.go | 24 ++++- .../cmd/abctl/tui/sessions_title_test.go | 97 +++++++++++++++++-- 3 files changed, 163 insertions(+), 23 deletions(-) diff --git a/authbridge/cmd/abctl/tui/app.go b/authbridge/cmd/abctl/tui/app.go index 6ba3cf25b..98134cac4 100644 --- a/authbridge/cmd/abctl/tui/app.go +++ b/authbridge/cmd/abctl/tui/app.go @@ -451,14 +451,24 @@ type model struct { harvesting bool // pickerHarvested says the namespaces/pods panes have already harvested during this visit. // - // Cleared by the pane-change edge in the refreshTickMsg branch rather than at each site that - // assigns m.pane: there are five of those across two files, and a sixth added later would - // silently inherit "already harvested" from a previous visit. lastPane is what that edge - // compares against. + // Cleared by the arrival edge in the refreshTickMsg branch rather than at each site that + // assigns m.pane: there are two dozen of those across app.go and keys.go, and one added later + // would silently inherit "already harvested" from a previous visit. pickerShowing is what + // that edge compares against. pickerHarvested bool - // lastPane is the pane the previous refresh tick saw, for detecting a pane change without - // every pane switch having to announce itself. Only the picker re-harvest reads it. - lastPane paneID + // pickerShowing is whether the previous refresh tick found the picker on screen, for + // detecting ARRIVAL at it without every pane switch having to announce itself. Only the + // picker harvest reads it. + // + // A BOOL, NOT THE PREVIOUS paneID, for two reasons that point the same way. The budget it + // clears covers a visit, and a visit spans paneNamespaces and panePods both — tracking the + // exact pane made namespaces→pods→namespaces three visits and re-walked the transcript tree + // at each hop. And a paneID field cannot express "no pane yet" without the paneNone sentinel + // that previousPane and pipelineReturnPane each need a comment to explain: paneNamespaces is + // the zero value, so a zero-valued previous-pane field claims the picker model is already + // where it starts and eats that model's first arrival. False is the honest zero here — no + // tick has seen the picker yet — so both constructors are correct without seeding anything. + pickerShowing bool // untitledMisses counts consecutive sessions-pane harvests that named nothing, and backs the // next one off exponentially — see untitledBackoff. // @@ -882,6 +892,13 @@ func (m *model) backToPodsPane() { // versions registered. The next `P` press refetches. m.catalog = nil m.catalogTbl.SetRows(nil) + // The backoff describes THE POD BEING LEFT, so it must not price the next one's first + // harvest. Those misses were recorded against a session list that is now gone (m.sessions is + // cleared just above), and a different pod is a different set of sessions with a different + // chance of being nameable. Left behind, six fruitless harvests here mean the next pod's list + // waits the 3m cap before its first scan instead of untitledSettleDelay — measured, not + // supposed. + m.untitledMisses = 0 m.previousPane = paneNone // Same reason: a return pane recorded against the pod being left would send // the next `P`-then-esc back into a pane belonging to the previous connection. @@ -1123,6 +1140,13 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, nil case harvestedMsg: + // THE ONLY PLACE harvesting IS CLEARED, and harvestCmd always returns a harvestedMsg — + // on success, on a harvester error (which it swallows) and on a nil map alike — so the + // flag is held for exactly one in-flight scan. That total coverage is the invariant: + // any future path that can drop this message instead of delivering it latches + // harvesting = true and silently disables every later harvest, in both the picker and + // the sessions pane, for the life of the process. There is no watchdog. A harvest that + // cannot report must still send this message. m.harvesting = false // COUNT THE MISS BEFORE THE MERGE, since the merge is what would hide it. A harvest that // named nothing NEW leaves every unnamed row unnamed, so the settle gate stays satisfied @@ -1208,11 +1232,18 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, tea.Batch(m.fetchUsage(), usageTick(m.usage.tickGen)) case refreshTickMsg: - // A PANE CHANGE ENDS THE PICKER'S ONE-HARVEST-PER-VISIT BUDGET. Detected here, on the - // edge, so the five places that assign m.pane do not each have to remember to clear it — - // and so a sixth cannot quietly skip the harvest by inheriting a set flag. - if m.pane != m.lastPane { - m.lastPane = m.pane + // ARRIVING AT THE PICKER ENDS ITS ONE-HARVEST-PER-VISIT BUDGET. Detected here, on the + // edge, so the two dozen places that assign m.pane do not each have to remember to clear + // it — and so one added later cannot quietly skip the harvest by inheriting a set flag. + // + // THE EDGE IS INTO THE PICKER AS A WHOLE, not into either of its panes. A visit spans + // both: enter on a namespace goes to panePods and esc comes back, and keying the budget + // on raw pane equality made each of those hops a fresh visit — so drilling into a + // namespace and backing out re-walked the entire transcript tree per hop, which is the + // opposite of what one-per-visit is for. What matters is whether the operator is newly + // in the picker at all, so that is what the edge compares. + if showing := m.pane == paneNamespaces || m.pane == panePods; showing != m.pickerShowing { + m.pickerShowing = showing m.pickerHarvested = false } // In picker mode, skip the fetch — m.client may be nil after a @@ -1259,11 +1290,15 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // than short-circuiting it — returning early instead would trade the titles for the // 2s refresh of every other cell in the row. var harvestNow tea.Cmd + // ONE now FOR THE WHOLE DECISION, so the backoff and the settle test are provably + // answered about the same instant rather than two readings of the clock a few + // microseconds apart. + now := time.Now() if m.pane == paneSessions && m.harvest != nil && !m.harvesting && - time.Since(m.lastHarvest) >= untitledBackoff(m.untitledMisses) && - m.untitledSettled(time.Now()) { + now.Sub(m.lastHarvest) >= untitledBackoff(m.untitledMisses) && + m.untitledSettled(now) { m.harvesting = true - m.lastHarvest = time.Now() + m.lastHarvest = now harvestNow = harvestCmd(m.harvest) } // Refresh the pipeline view too while a pane that displays plugin diff --git a/authbridge/cmd/abctl/tui/session_metadata.go b/authbridge/cmd/abctl/tui/session_metadata.go index 0bf76d433..4cc9539a1 100644 --- a/authbridge/cmd/abctl/tui/session_metadata.go +++ b/authbridge/cmd/abctl/tui/session_metadata.go @@ -210,7 +210,27 @@ func (m *model) untitledSettled(now time.Time) bool { // value, so this is defence at the consumer rather than a live upstream bug — but this file // renders whatever is in that map, including what an older harvester or a hand-edited file left. func (m *model) sessionHasTitle(id string) bool { - return strings.TrimSpace(m.sessionTitle(id)) != "" + return !titleIsBlank(m.sessionTitle(id)) +} + +// titleIsBlank reports whether a title string would render as an empty TITLE cell. +// +// THE ONE DEFINITION OF "UNNAMED", extracted because there are two callers asking that question +// about different strings — sessionHasTitle about what the model already holds, and +// harvestNamedSomething about what a harvest just returned, which is not in the model yet and so +// cannot be reached through sessionTitle. Both have to agree with the cell, and an inline copy in +// the second one is exactly the predicate/cell drift sessionHasTitle's comment exists to prevent. +// +// Sanitises before trimming because the cell does. sanitizeLabel REPLACES control and BIDI runes +// with U+FFFD rather than stripping them, so a title of only control characters is not blank — +// it renders a visible glyph, and claiming it unnamed would re-harvest forever for a row that is +// already showing something. +// +// Sanitising a string sessionTitle already sanitised is a no-op, not a second pass with different +// meaning: sanitizeLabel is idempotent — U+FFFD matches none of its cases and falls through — so +// the caller does not have to know which of the two paths got there first. +func titleIsBlank(title string) bool { + return strings.TrimSpace(sanitizeLabel(title)) == "" } // harvestNamedSomething reports whether a finished harvest named a session that is ON SCREEN and @@ -231,7 +251,7 @@ func (m *model) harvestNamedSomething(meta map[string]SessionMetadata) bool { if m.sessionHasTitle(sess.ID) { continue } - if strings.TrimSpace(sanitizeLabel(meta[sess.ID].Title)) != "" { + if !titleIsBlank(meta[sess.ID].Title) { return true } } diff --git a/authbridge/cmd/abctl/tui/sessions_title_test.go b/authbridge/cmd/abctl/tui/sessions_title_test.go index 75040757c..4a5061272 100644 --- a/authbridge/cmd/abctl/tui/sessions_title_test.go +++ b/authbridge/cmd/abctl/tui/sessions_title_test.go @@ -1,6 +1,7 @@ package tui import ( + "context" "encoding/json" "math/rand" "os" @@ -1462,13 +1463,16 @@ func TestUntitledBackoff_DoublesAndIsBounded(t *testing.T) { // TestPicker_HarvestsAtMostOncePerVisit pins the picker to one tree walk per visit. // // The namespaces/pods panes hold no session rows, so nothing there can tell a fruitless walk from -// a useful one and the interval alone would re-walk the tree for as long as the operator sits -// there. One scan answers what the pane needs: a session started elsewhere is named by the time -// they scroll to it. +// a useful one and an interval alone would re-walk the tree for as long as the operator sits +// there. One scan is what idle time is worth spending: a session started elsewhere is named by +// the time they scroll to it. func TestPicker_HarvestsAtMostOncePerVisit(t *testing.T) { m := newTitleModel(t, map[string]SessionMetadata{}, "s1") m.pane = paneNamespaces - m.lastPane = paneNamespaces + // pickerShowing = true, so the arrival edge has already been spent and what the ticks below + // exercise is the budget rather than the edge. TestPicker_HarvestsOnFirstTickFromConstructor + // covers the arrival itself, from an unseeded model. + m.pickerShowing = true called := 0 m.harvest = func() (map[string]SessionMetadata, error) { called++ @@ -1489,8 +1493,24 @@ func TestPicker_HarvestsAtMostOncePerVisit(t *testing.T) { t.Errorf("a second tick in the same visit harvested again (%d calls)", called) } - // Leaving and returning is a new visit, detected on the pane-change edge rather than by - // every assignment to m.pane announcing itself. + // DRILLING IN IS THE SAME VISIT. enter on a namespace moves to panePods and esc comes back; + // keying the budget on the exact pane made each hop a new visit and re-walked the whole + // transcript tree per keystroke. The operator never left the picker, so nothing should rescan. + m.pane = panePods + _, hop := m.Update(refreshTickMsg(time.Now())) + runBatch(t, hop) + if called != 1 { + t.Errorf("the namespaces->pods hop re-harvested (%d calls, want 1)", called) + } + m.pane = paneNamespaces + _, back := m.Update(refreshTickMsg(time.Now())) + runBatch(t, back) + if called != 1 { + t.Errorf("the pods->namespaces hop re-harvested (%d calls, want 1)", called) + } + + // Leaving the picker ALTOGETHER and returning is a new visit, detected on the arrival edge + // rather than by every assignment to m.pane announcing itself. m.pane = paneSessions m.Update(refreshTickMsg(time.Now())) m.pane = paneNamespaces @@ -1501,6 +1521,63 @@ func TestPicker_HarvestsAtMostOncePerVisit(t *testing.T) { } } +// TestPicker_HarvestsOnFirstTickFromConstructor pins the arrival harvest for the visit that +// matters most: the first one, on a model straight from newPickerModel. +// +// THE ZERO VALUE HAS TO BE RIGHT HERE, and it was not when this state was a paneID. The picker +// model STARTS on paneNamespaces, so a previous-pane field zero-valuing to paneNamespaces (it is +// iota 0) recorded "already here" before any tick ran — the edge never fired, and the first visit +// to the picker harvested only because Init happens to scan separately. Every test that set the +// field by hand to match m.pane masked it. This one constructs the state the way the constructor +// leaves it and asserts the scan happens anyway. +func TestPicker_HarvestsOnFirstTickFromConstructor(t *testing.T) { + m := newTitleModel(t, map[string]SessionMetadata{}, "s1") + m.pane = paneNamespaces + // Deliberately NOT seeding pickerShowing: the whole point is that the constructor does not + // either, and the first tick must still see an arrival. + called := 0 + m.harvest = func() (map[string]SessionMetadata, error) { + called++ + return nil, nil + } + + _, cmd := m.Update(refreshTickMsg(time.Now())) + runBatch(t, cmd) + if called != 1 { + t.Fatalf("the first tick on a constructor-shaped picker model harvested %d times, want 1", called) + } + if !m.pickerShowing { + t.Error("pickerShowing was not recorded, so the next tick will re-harvest") + } +} + +// TestBackToPodsPane_ResetsTheBackoff pins the widened backoff to the pod it was measured on. +// +// untitledMisses prices the NEXT harvest, and those misses were recorded against a session list +// that back-out throws away (m.sessions is cleared in the same block). A different pod is a +// different set of sessions with a different chance of being nameable, so carrying the counter +// across means the new pod's list waits out the old pod's penalty — at the cap, 3 minutes before +// its first scan instead of the 5s settle delay. Every other field describing the old connection +// is cleared there; this one was missed. +func TestBackToPodsPane_ResetsTheBackoff(t *testing.T) { + m := newTitleModel(t, map[string]SessionMetadata{}, "s1") + // backToPodsPane derives a fresh context from parentCtx. + m.parentCtx, m.ctx = context.Background(), context.Background() + m.cancel = func() {} + // Six fruitless harvests is past the cap, so the failure is a 3m wait rather than a small one. + m.untitledMisses = 6 + if untitledBackoff(m.untitledMisses) != untitledBackoffCap { + t.Fatalf("fixture did not reach the cap: %v", untitledBackoff(m.untitledMisses)) + } + + m.backToPodsPane() + + if m.untitledMisses != 0 { + t.Errorf("untitledMisses = %d after backing out; the next pod inherits a %v delay", + m.untitledMisses, untitledBackoff(m.untitledMisses)) + } +} + // TestUntitledSettled_BlankAndUnknownRows pins what counts as unnamed and as settled. // // A title of " " is non-empty to Go and blank in the column, so a raw `!= ""` suppressed the @@ -1562,6 +1639,14 @@ func TestHarvestNamedSomething_JudgesOnlyRowsOnScreen(t *testing.T) { if m.harvestNamedSomething(map[string]SessionMetadata{"s1": {Title: " "}}) { t.Error("a blank title counted as naming a session") } + // SANITISED BEFORE JUDGING, which matters HERE and not in sessionHasTitle's cases: this is the + // one caller handing titleIsBlank a RAW harvest result, where sessionTitle has not already + // sanitised on the way in. A control character becomes U+FFFD and renders a visible glyph, so + // the row IS named — dropping the sanitize would call it blank and re-harvest forever for a + // row that is already showing something. + if !m.harvestNamedSomething(map[string]SessionMetadata{"s1": {Title: "\t"}}) { + t.Error("a control-only title is a visible glyph in the cell, so it names the row") + } if m.harvestNamedSomething(map[string]SessionMetadata{"old": {Title: "elsewhere"}}) { t.Error("a title for the wrong session counted as naming the row on screen") } From 86905b6f2207af88d7daa890795c28e6559afc50 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Wed, 23 Sep 2026 21:13:18 -0400 Subject: [PATCH 05/10] fix: Count Init's harvest against the picker's first visit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Init harvests unconditionally, and in picker mode that IS the first visit's arrival scan — the operator is already on paneNamespaces when it runs. It stamped lastHarvest and set harvesting but recorded nothing about the visit, so once harvestedMsg cleared harvesting the next tick found an unspent budget and walked the whole transcript tree a second time, about two seconds into startup. The interval deleted in the last commit was the only thing suppressing it. Both pickerHarvested and pickerShowing have to be set, not just the first: with only the budget spent, the first tick still reads "picker newly showing" and clears it again. Each half was mutation-checked separately and each alone leaves the double scan in place. The test calls Init, which is what TestPicker_HarvestsOnFirstTickFrom- Constructor structurally cannot — it starts from a constructor-shaped model and never runs startup, so it sees one scan either way. Also from review: - sessionLabel routed through titleIsBlank. It was the third consumer of "is this session named" and the only one still on a raw != "", so a whitespace-only title rendered " (id)" — a header indented by a title that displays nothing. Mutation-checked. - untitledSettled's cost comment corrected. It claimed "one map lookup per row per tick"; sessionHasTitle goes through sanitizeLabel, which builds a string, so the steady state allocates per row per 2s tick and always walks the full list. Left linear on purpose — the rows are one pod's live sessions, and a cached flag is state to invalidate on every load and every merge — but the comment now says what it costs. - backToPodsPane notes that pickerHarvested/pickerShowing are left alone deliberately, being edge-derived rather than a second writer's job. - The pickerHarvestOnce comment block cut from ~25 lines to ~12. It was a doc comment naming a declaration that does not exist, and it kept the retired interval's reasoning as a quotation; the live reasoning is enough. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/cmd/abctl/tui/app.go | 48 ++++++++-------- authbridge/cmd/abctl/tui/session_metadata.go | 21 ++++++- .../cmd/abctl/tui/sessions_title_test.go | 55 +++++++++++++++++++ 3 files changed, 99 insertions(+), 25 deletions(-) diff --git a/authbridge/cmd/abctl/tui/app.go b/authbridge/cmd/abctl/tui/app.go index 98134cac4..09a597486 100644 --- a/authbridge/cmd/abctl/tui/app.go +++ b/authbridge/cmd/abctl/tui/app.go @@ -184,30 +184,19 @@ const localProbeTimeout = 2 * time.Second // stub sessions would otherwise linger in the TUI. const refreshInterval = 2 * time.Second -// pickerHarvestOnce records that the NAMESPACES and PODS panes harvest ONCE PER VISIT, on -// arrival, with no interval at all. There is no constant here because there is no longer a -// cadence to name — the visit itself is the trigger. +// The NAMESPACES and PODS panes harvest ONCE PER VISIT, on arrival, with no interval — so there +// is no constant to declare here. The visit is the trigger; pickerHarvested and pickerShowing +// carry the state, and Init spends the first visit's budget. // -// OPPORTUNISTIC, NOT NEEDED BY THE PANE. This is the reasoning that removed the interval, and -// it is not the reasoning the interval was written under. The picker is rarely visited, and the -// only reason to walk a transcript tree from it is that the system is otherwise idle waiting for -// someone to choose a pod — so the scan is work taken while nothing else wants the machine, not -// work the pane depends on. Idle-time work belongs at the moment you arrive; pacing it on a -// clock set by some earlier harvest gets the timing exactly backwards. +// The scan is OPPORTUNISTIC, not something the pane needs: the picker is rarely visited, and the +// reason to walk a transcript tree from it is that the machine is idle waiting for someone to +// choose a pod. Idle-time work belongs at the moment you arrive, so a clock set by some earlier +// harvest is the wrong pacing for it — a 3-minute interval used to sit here, and once the +// per-visit cap existed it could only suppress the one scan each visit was allowed. // -// A 3-MINUTE INTERVAL USED TO GUARD THIS, and once pickerHarvested existed it did nothing but -// SUPPRESS the single scan each visit was allowed: entering the picker less than three minutes -// after any other harvest skipped the visit's only walk, with nothing left to retry it until the -// operator left and came back after the clock aged out. An earlier version of this paragraph -// justified the interval by saying a harvest is expensive enough that polling it at the 2s -// session cadence would re-stat the whole tree ninety times a minute — true, and the reason the -// interval was right when it was the only thing limiting repeats. pickerHarvested limits them -// now, so the clock only ever cost a scan. -// -// The sessions LIST harvests on entirely separate terms — see untitledSettleDelay and -// untitledBackoff — because it gains a row whenever a session appears and can tell an unnamed -// row from a named one. These panes hold no session rows at all, which is why one scan per visit -// is the most they can sensibly ask for. +// The sessions LIST harvests on separate terms — see untitledSettleDelay and untitledBackoff — +// because it can tell an unnamed row from a named one. These panes hold no session rows, which +// is why one scan per visit is the most they can sensibly ask for. // untitledSettleDelay is how long a session must be quiet before an unnamed row triggers a // re-harvest, and it is the whole reason this poll is affordable. @@ -892,6 +881,10 @@ func (m *model) backToPodsPane() { // versions registered. The next `P` press refetches. m.catalog = nil m.catalogTbl.SetRows(nil) + // pickerHarvested and pickerShowing are deliberately NOT reset here. They are edge-derived: + // the next refresh tick sees the pane is the picker, finds pickerShowing false, and clears the + // budget itself. Resetting them here would be a second writer of state that already has one. + // // The backoff describes THE POD BEING LEFT, so it must not price the next one's first // harvest. Those misses were recorded against a session list that is now gone (m.sessions is // cleared just above), and a different pod is a different set of sessions with a different @@ -991,6 +984,17 @@ func (m *model) Init() tea.Cmd { if m.pane == paneNamespaces { // Picker mode — load agents, then idle until user picks a pod. m.loading = true + // INIT'S HARVEST *IS* THE FIRST VISIT'S ARRIVAL HARVEST, so record the arrival here: the + // operator is already on paneNamespaces when this runs, and the picker's rule is one walk + // per visit. Without this the first tick walked the whole transcript tree a SECOND time + // about two seconds into startup. The retired interval was what used to hide that. + // + // BOTH FIELDS, because either alone leaves the double scan in place. pickerHarvested is + // the spent budget; pickerShowing is what stops the first tick from reading this as a + // fresh arrival into the picker and clearing that budget again. They are one fact — "the + // picker is showing and its scan is done" — and Init is where it first becomes true. + m.pickerHarvested = true + m.pickerShowing = true return tea.Batch(loadAgentsCmd(m.ctx, m.lister), harvestCmd(m.harvest)) } return tea.Batch(m.initSessionView(), harvestCmd(m.harvest)) diff --git a/authbridge/cmd/abctl/tui/session_metadata.go b/authbridge/cmd/abctl/tui/session_metadata.go index 4cc9539a1..58c7e9f2d 100644 --- a/authbridge/cmd/abctl/tui/session_metadata.go +++ b/authbridge/cmd/abctl/tui/session_metadata.go @@ -109,7 +109,10 @@ func loadSessionMetadataForModel() map[string]SessionMetadata { // unique — two sessions in the same directory get the same harvested title, so a title // alone would make them indistinguishable in a header. func (m *model) sessionLabel(id string) string { - if title := m.sessionTitle(id); title != "" { + // THROUGH titleIsBlank, like the other two consumers of "is this named". A raw != "" accepted + // a whitespace-only title and rendered " (id)" — a header padded by a title that shows + // nothing, which is worse than the bare id it would otherwise print. + if title := m.sessionTitle(id); !titleIsBlank(title) { return title + " (" + id + ")" } return id @@ -175,8 +178,20 @@ func harvestCmd(h HarvestFunc) tea.Cmd { // this cheap: without it every event on a still-unnamed session would trigger a scan, and // with it a busy session is harvested once, after it pauses. // -// Only sessions the metadata does NOT name are considered, so the steady state — every row -// titled — triggers nothing at all and costs one map lookup per row per tick. +// Only sessions the metadata does NOT name are considered, so the steady state — every row titled +// — triggers nothing at all. +// +// IT IS NOT FREE, THOUGH, and an earlier version of this comment claimed "one map lookup per row +// per tick", which undersells it: sessionHasTitle goes through sessionTitle, which calls +// sanitizeLabel, which builds a new string. So the steady state allocates once per row per 2s +// tick and always walks the whole list — the all-titled case is the one that cannot exit early, +// because the loop is looking for a row that is not there. +// +// Left as a linear walk deliberately. The rows here are one pod's live sessions, a handful in +// practice against the ~180 in the metadata file, and the alternative — a cached "any untitled" +// flag — is a second piece of state to invalidate on every sessionsLoadedMsg and every harvest +// merge, which is how the events map grew the bugs its own comments now document. The honest +// figure is in the comment; the optimisation waits for a profile that asks for it. func (m *model) untitledSettled(now time.Time) bool { for _, s := range m.sessions { if m.sessionHasTitle(s.ID) { diff --git a/authbridge/cmd/abctl/tui/sessions_title_test.go b/authbridge/cmd/abctl/tui/sessions_title_test.go index 4a5061272..7ee0bba80 100644 --- a/authbridge/cmd/abctl/tui/sessions_title_test.go +++ b/authbridge/cmd/abctl/tui/sessions_title_test.go @@ -291,6 +291,14 @@ func TestSessionLabelAndHeader(t *testing.T) { if got, want := m.sessionLabel("id-2"), "id-2"; got != want { t.Errorf("sessionLabel for an unharvested session = %q, want the bare id %q", got, want) } + // A WHITESPACE-ONLY TITLE IS UNNAMED HERE TOO, on the same terms as the TITLE cell and the + // harvest gate. A raw != "" accepted it and produced " (id-3)": a header indented by a + // title that displays nothing, which reads as a rendering fault rather than as a session + // nobody has named. + m.sessionsData["id-3"] = SessionMetadata{Title: " "} + if got, want := m.sessionLabel("id-3"), "id-3"; got != want { + t.Errorf("sessionLabel for a blank title = %q, want the bare id %q", got, want) + } if got, want := m.sessionHeader("id-1", ""), "abctl · fix the parser (id-1)"; got != want { t.Errorf("events header = %q, want %q", got, want) } @@ -1696,3 +1704,50 @@ func TestHarvestedMsg_UnscoreableHarvestsDoNotMoveTheBackoff(t *testing.T) { t.Errorf("a harvest that named a visible row left untitledMisses = %d, want 0", m.untitledMisses) } } + +// TestPicker_InitHarvestCountsAgainstTheVisit pins Init's own scan as the visit's one scan. +// +// Init harvests unconditionally, and in picker mode that IS the arrival harvest for the first +// visit — the operator is already on paneNamespaces when it runs. It stamped lastHarvest and set +// harvesting but not pickerHarvested, so once harvestedMsg cleared harvesting the very next tick +// found an unspent budget and walked the whole transcript tree a second time, about two seconds +// into startup. The deleted interval was the only thing suppressing that. +// +// Calls Init, which is what TestPicker_HarvestsOnFirstTickFromConstructor cannot: that test +// starts from a constructor-shaped model and never runs startup, so it sees one scan either way. +func TestPicker_InitHarvestCountsAgainstTheVisit(t *testing.T) { + m := newTitleModel(t, map[string]SessionMetadata{}, "s1") + m.pane = paneNamespaces + m.ctx = context.Background() + // Init batches loadAgentsCmd alongside the harvest, and runBatch dispatches the whole batch. + m.lister = &fakeLister{namespaces: fixtureNamespaces} + called := 0 + m.harvest = func() (map[string]SessionMetadata, error) { + called++ + return nil, nil + } + + runBatch(t, m.Init()) + if called != 1 { + t.Fatalf("Init harvested %d times, want 1", called) + } + if !m.pickerHarvested { + t.Error("Init did not spend the visit's budget, so the next tick will rescan") + } + m.Update(harvestedMsg{}) + + // The first tick after startup must not walk the tree again: the operator has not left the + // picker, and Init already scanned on their behalf. + _, cmd := m.Update(refreshTickMsg(time.Now())) + runBatch(t, cmd) + if called != 1 { + t.Errorf("the first tick after Init re-harvested (%d calls, want 1)", called) + } + + // And not on the tick after that either. + _, again := m.Update(refreshTickMsg(time.Now())) + runBatch(t, again) + if called != 1 { + t.Errorf("a later tick in the same visit re-harvested (%d calls, want 1)", called) + } +} From dba4d8dfe5a81787108f94f3e0b18f320412dc9f Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Wed, 23 Sep 2026 21:51:54 -0400 Subject: [PATCH 06/10] docs: Retire stale harvest comments, drop a dead argument MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comment-and-dead-code pass from review. One behaviour change, in a branch that could not execute. - The pipeline branch no longer passes harvestNow. It is assigned only under m.pane == paneSessions, and reaching that return requires panePluginDetail or panePipeline, so it was always nil. tea.Batch dropped it harmlessly; the problem was implying a combination that cannot occur. - "see pickerHarvestOnce" pointed at an identifier that exists nowhere. That name was a doc-comment heading trimmed in the previous commit; the reference now points at the surviving prose. - The picker branch opened with "RE-HARVEST WHILE THE PICKER IS OPEN … keyed off the existing refresh ticker" and then contradicted itself a paragraph later with "no interval here on purpose". Rewritten so the ticker is described as how the scan is delivered, not what paces it. - lastHarvest's field comment called itself "the picker's re-harvest cadence". The picker has no cadence now; the sessions-pane backoff is the only reader. It also now records the coupling the PR body discloses: every path stamps this, including the two that never read it, so a picker or startup scan can hold off the sessions pane's first settle-triggered harvest by up to one settle delay. The PR body is not in the tree; this is. - Init's comment cited the deleted 3-minute interval. app.go's other mention is deliberately historical and stays. - backToPodsPane's note read as if finding pickerShowing false were what clears the budget, rather than the inequality on the tick edge. Two review items were checked and NOT changed, both by execution: - titleIsBlank's sanitise-then-trim order is deliberate, and reversing it as suggested would be a regression. The cell renders sessionTitle, which is sanitised and never trimmed, so a "\t" title paints U+FFFD — a visible glyph. sanitizeLabel(TrimSpace(title)) would call that row unnamed while the screen shows something, which is the predicate/cell divergence this helper exists to prevent, and it would re-harvest forever for a row that is already displaying a character. The reasoning is now in the comment, including which runes the two orders differ on. - sessionTitleCell's raw title == "" is a fast path to skip truncating an empty string, not a competing blankness test: " " falls through it and returns " ". Predicate and cell were verified to agree on "", " ", " ", "\t" and prose. The claim corrected is my own comment's, which said the cell calls the shared predicate; it does not, and the agreement is by construction. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/cmd/abctl/tui/app.go | 56 +++++++++++++------- authbridge/cmd/abctl/tui/session_metadata.go | 33 ++++++++---- 2 files changed, 61 insertions(+), 28 deletions(-) diff --git a/authbridge/cmd/abctl/tui/app.go b/authbridge/cmd/abctl/tui/app.go index 09a597486..612d5b6f8 100644 --- a/authbridge/cmd/abctl/tui/app.go +++ b/authbridge/cmd/abctl/tui/app.go @@ -433,7 +433,17 @@ type model struct { sessionsData map[string]SessionMetadata // harvest refreshes sessionsData in the background once the UI is up. Nil disables it. harvest HarvestFunc - // lastHarvest is when the most recent harvest was STARTED, for the picker's re-harvest cadence. + // lastHarvest is when the most recent harvest was STARTED. Read by ONE caller: the sessions + // pane's backoff gate, which asks whether untitledBackoff has elapsed since then. The picker + // no longer has a cadence to measure — it harvests on arrival — so nothing there reads this. + // + // STAMPED BY EVERY PATH, THOUGH, INCLUDING THE ONES THAT NEVER READ IT, and that is a real + // coupling rather than a tidy one: a picker scan on arrival, and Init's startup scan, both + // move this stamp, so the sessions pane's first settle-triggered harvest can be held off for + // up to one untitledSettleDelay after the operator picks a pod. Bounded at 5s, and defensible + // — the tree was just walked, so there is little to gain from walking it again — but it means + // the two paths are not as independent as their separate triggers suggest. A dedicated + // lastSessionsHarvest would decouple them; deliberately not done here. lastHarvest time.Time // harvesting guards against stacking: a harvest walks a transcript tree, and a second pass while // the first is in flight would duplicate the work and race its own write of the metadata file. @@ -881,9 +891,10 @@ func (m *model) backToPodsPane() { // versions registered. The next `P` press refetches. m.catalog = nil m.catalogTbl.SetRows(nil) - // pickerHarvested and pickerShowing are deliberately NOT reset here. They are edge-derived: - // the next refresh tick sees the pane is the picker, finds pickerShowing false, and clears the - // budget itself. Resetting them here would be a second writer of state that already has one. + // pickerHarvested and pickerShowing are deliberately NOT reset here. They are edge-derived: the + // next refresh tick recomputes "is the picker showing", finds it disagrees with the recorded + // pickerShowing, and clears the budget on that inequality. Resetting them here would make this + // a second writer of state that already has exactly one. // // The backoff describes THE POD BEING LEFT, so it must not price the next one's first // harvest. Those misses were recorded against a session list that is now gone (m.sessions is @@ -974,9 +985,9 @@ func (m *model) Init() tea.Cmd { // disk names every session the last run saw, so a harvest only ever ADDS titles — and // reading a large ~/.claude takes about as long as everything else at startup put // together. Batched rather than sequenced so neither waits on the other. - // Stamped here, not on arrival: the cadence is measured from when a harvest STARTS, so leaving it - // zero would make the first tick three minutes later look overdue regardless of when the initial - // harvest actually ran. + // Stamped here, not on arrival: the sessions pane's backoff is measured from when a harvest + // STARTS, so leaving it zero would make its first tick look overdue by the whole age of the + // clock regardless of when this harvest actually ran. if m.harvest != nil { m.harvesting = true m.lastHarvest = time.Now() @@ -1254,19 +1265,22 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // back-out. Keep the ticker alive so it's ready when the user // re-enters a session. if m.pane == paneNamespaces || m.pane == panePods { - // RE-HARVEST WHILE THE PICKER IS OPEN, so a session started in another terminal is named - // by the time the user scrolls to it. This is the one pane where someone may sit for - // minutes with nothing else refreshing the titles on screen. + // HARVEST ON ARRIVAL, EXACTLY ONCE PER VISIT, so a session started in another terminal + // is named by the time the user scrolls to it. This is the one pane where someone may + // sit for minutes with nothing else refreshing the titles on screen. + // + // pickerHarvested is the spent budget and pickerShowing is the arrival edge; Init sets + // both for the first visit. There is no interval on purpose — the scan is opportunistic + // idle-time work, so it belongs at the moment of arrival rather than on a clock some + // earlier harvest set. See the comment above refreshInterval. // - // Keyed off the existing refresh ticker rather than a second tea.Tick: one timer is - // easier to reason about than two with different periods, and this branch already - // returns on every tick. + // Riding the existing refresh ticker rather than a second tea.Tick: one timer is easier + // to reason about than two, and this branch already returns on every tick. That is how + // the scan is DELIVERED, not what paces it. // - // EXACTLY ONCE PER VISIT, ON ARRIVAL, which is what pickerHarvested tracks — there is - // no interval here on purpose; see pickerHarvestOnce. These panes hold no session rows, - // so nothing here can tell a fruitless walk from a useful one, and the sessions pane's - // backoff cannot help because this path does not consult it. Cleared on entry to the - // pane, so coming back later harvests again. + // These panes hold no session rows, so nothing here can tell a fruitless walk from a + // useful one, and the sessions pane's backoff cannot help because this path does not + // consult it. One walk per visit is the most they can sensibly ask for. if m.harvest != nil && !m.harvesting && !m.pickerHarvested { m.harvesting = true m.pickerHarvested = true @@ -1315,7 +1329,11 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // requests and keep adding one every tick. if (m.pane == panePluginDetail || m.pane == panePipeline) && !m.pipelineFetching { m.pipelineFetching = true - return m, tea.Batch(m.loadSessionsCmd(), m.loadPipelineCmd(), refreshTickCmd(), harvestNow) + // harvestNow is deliberately absent: it is only ever set under m.pane == paneSessions, + // and reaching here requires panePluginDetail or panePipeline, so passing it would + // imply a combination that cannot occur. tea.Batch would drop the nil harmlessly — + // the point is not to suggest otherwise to the next reader. + return m, tea.Batch(m.loadSessionsCmd(), m.loadPipelineCmd(), refreshTickCmd()) } return m, tea.Batch(m.loadSessionsCmd(), refreshTickCmd(), harvestNow) diff --git a/authbridge/cmd/abctl/tui/session_metadata.go b/authbridge/cmd/abctl/tui/session_metadata.go index 58c7e9f2d..b1bf11b99 100644 --- a/authbridge/cmd/abctl/tui/session_metadata.go +++ b/authbridge/cmd/abctl/tui/session_metadata.go @@ -230,16 +230,31 @@ func (m *model) sessionHasTitle(id string) bool { // titleIsBlank reports whether a title string would render as an empty TITLE cell. // -// THE ONE DEFINITION OF "UNNAMED", extracted because there are two callers asking that question -// about different strings — sessionHasTitle about what the model already holds, and -// harvestNamedSomething about what a harvest just returned, which is not in the model yet and so -// cannot be reached through sessionTitle. Both have to agree with the cell, and an inline copy in -// the second one is exactly the predicate/cell drift sessionHasTitle's comment exists to prevent. +// THE ONE DEFINITION OF "UNNAMED" AMONG THE PREDICATES, extracted because three callers ask that +// question about different strings — sessionHasTitle about what the model already holds, +// sessionLabel about the same for a header, and harvestNamedSomething about what a harvest just +// returned, which is not in the model yet and so cannot be reached through sessionTitle. An +// inline copy in any of them is the drift sessionHasTitle's comment exists to prevent. // -// Sanitises before trimming because the cell does. sanitizeLabel REPLACES control and BIDI runes -// with U+FFFD rather than stripping them, so a title of only control characters is not blank — -// it renders a visible glyph, and claiming it unnamed would re-harvest forever for a row that is -// already showing something. +// THE CELL DOES NOT CALL THIS, and the claim that it does was overstated. sessionTitleCell tests +// a raw title == "" as a fast path to skip truncating an empty string; it does not judge +// blankness, and a " " title falls through it and is returned as " ". So the two AGREE in +// behaviour on every input — verified across "", " ", " ", "\t" and ordinary prose — but by +// construction rather than by sharing this function. If that fast path ever becomes a real +// blankness test, it should route through here. +// +// SANITISES BEFORE TRIMMING, in that order, because that is the order the cell applies them: it +// renders sessionTitle, which is sanitizeLabel'd, and nothing trims afterwards. sanitizeLabel +// REPLACES control and BIDI runes with U+FFFD rather than stripping them, so a title of "\t" or +// "\n" is NOT blank here — the cell shows "�", a visible glyph, and a predicate calling that row +// unnamed would re-harvest forever for a row that is already displaying something. +// +// Reversing the two — sanitizeLabel(TrimSpace(title)) — is the tempting reading, since it makes +// "\t" answer "blank" the way a human skimming the source expects. It is wrong for this +// predicate: TrimSpace would strip the tab before sanitizeLabel could turn it into the glyph the +// cell actually paints, so the predicate would disagree with the screen. That disagreement is the +// one thing this helper exists to prevent. (Only tab/newline-class runes differ between the two +// orders; NUL and the BIDI controls are not whitespace, so TrimSpace never reaches them.) // // Sanitising a string sessionTitle already sanitised is a no-op, not a second pass with different // meaning: sanitizeLabel is idempotent — U+FFFD matches none of its cases and falls through — so From 474e78b15591e5629f5e68faee01ef5d0ee3ca9f Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Thu, 24 Sep 2026 06:59:41 -0400 Subject: [PATCH 07/10] fix: Stop one unnameable session penalising every new one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit untitledMisses is a single model-wide counter, but what it describes — "asking about this row is fruitless" — is per-row. A session with no transcript under the agent's config dir can never be named, so it drives the counter to untitledBackoffCap. A genuinely new unnamed session appearing afterwards then inherited that 3m wait for its FIRST title, though nothing had ever been asked about it. That is #1109's symptom with a longer fuse, and it is the ordinary case: an operator watching a pod gets a new session while an unnameable one is on screen. Neither session-arrival path reset it — sessionsLoadedMsg replaces the slice and returns, and the stream stub only bumps UpdatedAt. Reset the counter when an unnamed row appears that the last scoring did not see, tracked in untitledCounted. The set holds unnamed ids only and is rebuilt at each scoring, so ids drop out when their row gains a title or leaves the list, and a re-created session id counts as new. Kept the backoff global rather than keying it per session id: the harvest is one tree walk for ALL sessions, so per-row deadlines would still share a single scan and the first row due would set the cadence anyway. What the defect costs is a stale penalty carried onto a fresh row, which an arrival reset ends without inventing a per-row schedule. Measured before: 8 misses, 3m backoff, and a settled new row refused a harvest 6s later. After: the arrival resets to 5s and the gate fires. Four new tests, each mutation-checked. Two existing tests were counting from a cold model whose first harvest now resets on arrival; both warm the set first, so each still measures the widening it was written for. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/cmd/abctl/tui/app.go | 69 ++++++++++++++++++++++++++++++--- 1 file changed, 63 insertions(+), 6 deletions(-) diff --git a/authbridge/cmd/abctl/tui/app.go b/authbridge/cmd/abctl/tui/app.go index 612d5b6f8..416b14520 100644 --- a/authbridge/cmd/abctl/tui/app.go +++ b/authbridge/cmd/abctl/tui/app.go @@ -479,11 +479,42 @@ type model struct { // // Reset on any harvest that named something, so a tree that starts producing titles returns to // the fast cadence immediately rather than staying penalised for earlier silence. + // + // AND RESET WHEN AN UNNAMED SESSION ARRIVES THAT WAS NOT HERE BEFORE — see untitledCounted, + // which is what makes that detectable. Without it this counter is model-wide while the thing + // it is meant to describe is per-session: one row that can never be named drives it to the + // untitledBackoffCap, and a genuinely new session appearing afterwards inherits that 3m wait + // for its first title. That is #1109's symptom with a longer fuse, and it is the ordinary + // case — an operator watching a pod gets a new session while an unnameable one is on screen. untitledMisses int - eventCt uint64 // monotonic counter - lastTick time.Time - lastCt uint64 - rate float64 + // untitledCounted is the set of session ids that were unnamed at the last harvest scoring, + // so the next one can tell a NEW unnamed row from the same unnameable row asked again. + // + // WHY A SET AND NOT A PER-SESSION BACKOFF. Keying untitledMisses by session id is the more + // literal reading of "the counter is not per-session", and it is the bigger change: the gate + // would have to pick a deadline across rows, which is a policy question this PR does not need + // to answer — the harvest is one tree walk for ALL sessions, so per-session deadlines would + // still share a single scan and the first row due would set the cadence for everyone. What + // the defect actually costs is a stale penalty carried onto a fresh row, and an arrival reset + // ends that without inventing a per-row schedule. So the backoff stays global and describes + // "how fruitless has this LIST been lately", which is what a single tree walk can honour. + // + // HOLDS UNNAMED IDS ONLY, not every id seen. A session that already has a title is not what + // the settle gate or the backoff is about, and admitting it here would mean a row LOSING its + // title later (a hand-edited metadata file, a merge that blanks one) read as "not new" and + // skipped the reset. Membership answers exactly one question — was this row already counted + // against the backoff as unnameable — so only rows that were counted belong in it. + // + // REBUILT AT EACH SCORING RATHER THAN ADDED TO, so ids drop out when their session leaves the + // list or gains a title. An append-only set would grow for the life of the process and, worse, + // would remember a session that went away and came back as "already counted" — abctl's own + // docs note a session id can be re-created after eviction, and a returning id is a new row to + // an operator watching the pane. + untitledCounted map[string]bool + eventCt uint64 // monotonic counter + lastTick time.Time + lastCt uint64 + rate float64 // Connection status. connState connStateInfo @@ -1185,10 +1216,36 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // // UNSCOREABLE IS NEITHER, so the counter holds rather than resetting: a harvest nobody // could judge is no reason to believe the tree started producing titles either. + // + // AN UNNAMED ROW THIS SCORING HAS NOT SEEN BEFORE RESETS THE COUNTER, whatever the harvest + // found, because the accumulated penalty was earned by OTHER rows. A row that can never be + // named pins untitledMisses at the cap, and the next session to appear is a fresh question + // the tree has never been asked — making it wait out a 3m backoff earned by a different + // session is the bug. Ordered before the miss count so a tick that both gains a new row and + // fails to name anything resets rather than incrementing: the new row has not been tried + // yet, so there is no evidence against it to count. if len(m.sessions) > 0 { - if m.harvestNamedSomething(msg.meta) { + counted := make(map[string]bool, len(m.sessions)) + fresh := false + for _, sess := range m.sessions { + if m.sessionHasTitle(sess.ID) { + continue + } + counted[sess.ID] = true + if !m.untitledCounted[sess.ID] { + fresh = true + } + } + // ASSIGNED BEFORE THE BRANCH, so every scoreable tick records what it judged. Doing it + // only on one arm would leave the set describing some earlier tick, and the next new + // row would be compared against a stale snapshot. + m.untitledCounted = counted + switch { + case fresh: m.untitledMisses = 0 - } else { + case m.harvestNamedSomething(msg.meta): + m.untitledMisses = 0 + default: m.untitledMisses++ } } From 263b5ab9369a8da8aa10e0d905c44223936c3f3c Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Thu, 24 Sep 2026 06:59:53 -0400 Subject: [PATCH 08/10] fix: Do not strand a title on a pod clock that runs ahead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit untitledSettled compares the laptop's clock against UpdatedAt, which is stamped inside the pod (authlib/session/store.go) or carried on a streamed event. Nothing keeps the two in step: the pane reaches the store through a port-forward, Kubernetes does not synchronise node clocks, and a laptop that slept is the common way the gap grows. A pod clock ahead of the client gives a negative delta, which can never reach untitledSettleDelay — so that row's title never arrives. No error, no log, just a permanently blank TITLE cell, and the skew has to exceed only 5s to do it. The IsZero guard does not cover this. Treat a future UpdatedAt as settled. That is the safe direction: harvesting early costs one tree walk that the backoff then widens, against a title that otherwise never comes at all. Clamped rather than corrected, because there is nothing to correct against — both timestamps on SessionSummary are server-stamped, so the response carries no client-anchored instant to measure the offset from. Inventing one would be a second clock model for a pane whose AGE column already tolerates the same skew: relTime renders a negative delta as "just now". The asymmetry is why the fix is scoped here — a wrong AGE is visibly wrong for one tick, a wrong settle answer is invisible and permanent. Also carries the tests for the preceding commit, which share this file. Both clamp mutations checked: dropping the negative clause and widening it to swallow the busy case are each caught. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/cmd/abctl/tui/session_metadata.go | 26 ++- .../cmd/abctl/tui/sessions_title_test.go | 169 ++++++++++++++++++ 2 files changed, 194 insertions(+), 1 deletion(-) diff --git a/authbridge/cmd/abctl/tui/session_metadata.go b/authbridge/cmd/abctl/tui/session_metadata.go index b1bf11b99..8702ed693 100644 --- a/authbridge/cmd/abctl/tui/session_metadata.go +++ b/authbridge/cmd/abctl/tui/session_metadata.go @@ -181,6 +181,9 @@ func harvestCmd(h HarvestFunc) tea.Cmd { // Only sessions the metadata does NOT name are considered, so the steady state — every row titled // — triggers nothing at all. // +// THE QUIET TEST SPANS TWO CLOCKS and tolerates them disagreeing in the safe direction; the +// reasoning is at the comparison itself. +// // IT IS NOT FREE, THOUGH, and an earlier version of this comment claimed "one map lookup per row // per tick", which undersells it: sessionHasTitle goes through sessionTitle, which calls // sanitizeLabel, which builds a new string. So the steady state allocates once per row per 2s @@ -204,7 +207,28 @@ func (m *model) untitledSettled(now time.Time) bool { if s.UpdatedAt.IsZero() { continue } - if now.Sub(s.UpdatedAt) >= untitledSettleDelay { + // TWO CLOCKS, NOT ONE, and this subtraction is the only place in the pane where that + // costs anything. now is the laptop's; UpdatedAt was stamped inside the pod + // (authlib/session/store.go, sess.UpdatedAt = now) or carried on a streamed event, and + // the two are reached through a kubectl port-forward with nothing keeping them in step. + // Kubernetes does not synchronise node clocks, and a laptop that slept is the common + // way this gets large. + // + // A FUTURE UpdatedAt IS A BROKEN CLOCK, NOT A SETTLED SESSION. Pod ahead of client gives + // a negative delta, which can never reach untitledSettleDelay, so that row's title never + // arrives — no error, no log, just a permanently blank TITLE cell, and the skew has to + // exceed only 5s to do it. Treating it as settled instead is the safe direction: the + // cost of harvesting early is one wasted tree walk that the backoff then widens, against + // a title that otherwise never comes at all. + // + // Clamped rather than corrected, because there is nothing to correct against. Both + // timestamps on SessionSummary are server-stamped, so the response carries no + // client-anchored instant to measure the offset from, and inventing one (first-seen-at, + // per row) would be a second clock model for a pane whose AGE column already tolerates + // the same skew — relTime renders a negative delta as "just now" and moves on. The + // asymmetry is the point: a wrong AGE is visibly wrong for one tick, while a wrong + // settle answer is invisible and permanent. + if quiet := now.Sub(s.UpdatedAt); quiet < 0 || quiet >= untitledSettleDelay { return true } } diff --git a/authbridge/cmd/abctl/tui/sessions_title_test.go b/authbridge/cmd/abctl/tui/sessions_title_test.go index 7ee0bba80..b3efe66b4 100644 --- a/authbridge/cmd/abctl/tui/sessions_title_test.go +++ b/authbridge/cmd/abctl/tui/sessions_title_test.go @@ -1406,6 +1406,15 @@ func TestSessionsPane_BacksOffFruitlessHarvests(t *testing.T) { m.sessions = []session.SessionSummary{{ID: "s1", UpdatedAt: time.Now().Add(-time.Hour)}} m.harvest = func() (map[string]SessionMetadata, error) { return nil, nil } + // SPEND THE ARRIVAL RESET FIRST. s1 is unnamed and this model has never scored it, so the + // first harvest resets rather than counting — see untitledCounted. That is deliberate (a row + // nobody has asked about yet is not evidence that asking is fruitless) and it is not what + // this test is about, so get past it before measuring the widening. + m.Update(harvestedMsg{}) + if m.untitledMisses != 0 { + t.Fatalf("first harvest on a never-scored row should reset, got %d", m.untitledMisses) + } + // A harvest that names nothing widens the wait. for want := 1; want <= 3; want++ { m.lastHarvest = time.Now().Add(-untitledBackoffCap) @@ -1695,6 +1704,9 @@ func TestHarvestedMsg_UnscoreableHarvestsDoNotMoveTheBackoff(t *testing.T) { // what counting means. m = newTitleModel(t, map[string]SessionMetadata{}, "s1") m.sessions = []session.SessionSummary{{ID: "s1", UpdatedAt: time.Now()}} + // Twice: the first harvest meets s1 and resets on arrival, the second is the miss being + // measured. Scoring a row the model has never seen unnamed is a reset by design. + m.Update(harvestedMsg{meta: map[string]SessionMetadata{"other": {Title: "not on screen"}}}) m.Update(harvestedMsg{meta: map[string]SessionMetadata{"other": {Title: "not on screen"}}}) if m.untitledMisses != 1 { t.Errorf("a fruitless harvest with rows present left untitledMisses = %d, want 1", m.untitledMisses) @@ -1751,3 +1763,160 @@ func TestPicker_InitHarvestCountsAgainstTheVisit(t *testing.T) { t.Errorf("a later tick in the same visit re-harvested (%d calls, want 1)", called) } } + +// TestUntitledMisses_NewSessionClearsAnotherRowsPenalty is the #1109 symptom with a longer fuse. +// +// untitledMisses is one model-wide counter, but the thing it describes — "asking about this row is +// fruitless" — is per-row. A session with no transcript under the agent's config dir can never be +// named, so it drives the counter to untitledBackoffCap. Before the arrival reset, a genuinely new +// unnamed session appearing afterwards inherited that 3m wait for its FIRST title: the backoff gate +// refused it, though nothing had ever been asked about it. +// +// DRIVES THE REAL HANDLER, not the fields. The reset has to happen where the misses are counted, so +// a test that set untitledMisses by hand would pass against a fix placed anywhere at all. +func TestUntitledMisses_NewSessionClearsAnotherRowsPenalty(t *testing.T) { + // One row that no harvest will ever name. + m := newTitleModel(t, map[string]SessionMetadata{}, "unnameable") + empty := map[string]SessionMetadata{} + + // Eight fruitless harvests on that row. Past the cap, so the failure is a 3m wait. + for i := 0; i < 8; i++ { + m.Update(harvestedMsg{meta: empty}) + } + if got := untitledBackoff(m.untitledMisses); got != untitledBackoffCap { + t.Fatalf("fixture did not reach the cap: misses=%d backoff=%v", m.untitledMisses, got) + } + + // A new session appears — the ordinary case, an operator watching a pod while an unnameable + // row sits on screen. The poll path replaces the slice wholesale; both rows are unnamed. + m.sessions = append(m.sessions, session.SessionSummary{ID: "brandnew", UpdatedAt: time.Now()}) + + // The next harvest still names nothing: the new session's transcript is not readable yet, + // which is exactly when its first retry matters most. + m.Update(harvestedMsg{meta: empty}) + + if m.untitledMisses != 0 { + t.Errorf("untitledMisses = %d after a previously-unseen unnamed session arrived; "+ + "its first title waits %v, earned by a different row", + m.untitledMisses, untitledBackoff(m.untitledMisses)) + } +} + +// TestUntitledMisses_SameUnnameableRowKeepsBackingOff is the other half of the arrival reset. +// +// The reset keys on a row this scoring has not seen unnamed before. If it instead fired whenever +// any unnamed row was present, the backoff would be permanently reset and the loop it exists to +// break — a full transcript-tree walk every untitledSettleDelay for a title that is never coming — +// would be back with extra code. So: the same row asked again must still widen the wait. +func TestUntitledMisses_SameUnnameableRowKeepsBackingOff(t *testing.T) { + m := newTitleModel(t, map[string]SessionMetadata{}, "unnameable") + empty := map[string]SessionMetadata{} + + // FOUR HARVESTS, THREE MISSES. The first one meets this row for the first time and resets, + // which is the arrival reset working as intended rather than an off-by-one: a row that has + // never been asked about is not evidence that asking is fruitless. Only the repeats count. + for i := 0; i < 4; i++ { + m.Update(harvestedMsg{meta: empty}) + } + + if m.untitledMisses != 3 { + t.Errorf("untitledMisses = %d after 4 fruitless harvests on one unchanged row, want 3 "+ + "(backoff %v); the arrival reset is firing on a row it already counted", + m.untitledMisses, untitledBackoff(m.untitledMisses)) + } +} + +// TestUntitledMisses_ReturningSessionCountsAsNew pins that the set is rebuilt, not appended to. +// +// A row that leaves the list and comes back is a new row to an operator watching the pane, and +// abctl's own docs note a session id can be re-created after eviction. An append-only set would +// remember it as "already counted" and make its first title wait out a backoff earned before it +// went away. +func TestUntitledMisses_ReturningSessionCountsAsNew(t *testing.T) { + m := newTitleModel(t, map[string]SessionMetadata{}, "comesback") + empty := map[string]SessionMetadata{} + + for i := 0; i < 8; i++ { + m.Update(harvestedMsg{meta: empty}) + } + if got := untitledBackoff(m.untitledMisses); got != untitledBackoffCap { + t.Fatalf("fixture did not reach the cap: misses=%d backoff=%v", m.untitledMisses, got) + } + + // Gone: the poll returned a list without it. Scored while absent, so the set forgets it. + m.sessions = []session.SessionSummary{{ID: "other", UpdatedAt: time.Now()}} + m.sessionsData = map[string]SessionMetadata{"other": {Title: "named"}} + m.Update(harvestedMsg{meta: empty}) + + // Back again, still unnamed. + m.sessions = append(m.sessions, session.SessionSummary{ID: "comesback", UpdatedAt: time.Now()}) + m.Update(harvestedMsg{meta: empty}) + + if m.untitledMisses != 0 { + t.Errorf("untitledMisses = %d after a session returned to the list; "+ + "the set is remembering ids whose rows are gone", m.untitledMisses) + } +} + +// TestUntitledMisses_TitledRowsStayOutOfTheSet pins that only rows counted against the backoff are +// remembered. +// +// A row that already has a title is not what the backoff is about. Admitting it to the set would +// mean a row LOSING its title later — a hand-edited metadata file, a merge that blanks one — read +// as "not new" and skipped the reset, leaving it to wait out a penalty it never earned. +func TestUntitledMisses_TitledRowsStayOutOfTheSet(t *testing.T) { + m := newTitleModel(t, map[string]SessionMetadata{"s1": {Title: "known"}}, "s1", "unnameable") + empty := map[string]SessionMetadata{} + + for i := 0; i < 8; i++ { + m.Update(harvestedMsg{meta: empty}) + } + if got := untitledBackoff(m.untitledMisses); got != untitledBackoffCap { + t.Fatalf("fixture did not reach the cap: misses=%d backoff=%v", m.untitledMisses, got) + } + + // s1 loses its title. It is now an unnamed row that was never counted as one. + m.sessionsData = map[string]SessionMetadata{} + m.Update(harvestedMsg{meta: empty}) + + if m.untitledMisses != 0 { + t.Errorf("untitledMisses = %d after a titled row went blank; it is being treated as "+ + "already counted", m.untitledMisses) + } +} + +// TestUntitledSettled_FutureUpdatedAtIsNotQuietForever covers client/server clock skew. +// +// now is the laptop's clock; UpdatedAt was stamped in the pod. Nothing keeps them in step — the +// pane reaches the store through a port-forward, Kubernetes does not synchronise node clocks, and +// a laptop that slept is the ordinary way the gap gets large. With a plain now.Sub, a pod clock +// even 5s ahead makes the delta negative, which can never reach untitledSettleDelay: that row's +// title never arrives, with no error and no log. Treating a future stamp as settled costs one +// wasted tree walk that the backoff then widens. +func TestUntitledSettled_FutureUpdatedAtIsNotQuietForever(t *testing.T) { + now := time.Now() + + cases := []struct { + name string + upd time.Time + want bool + }{ + // The skew only has to exceed untitledSettleDelay to strand a row forever. + {"pod clock slightly ahead", now.Add(2 * untitledSettleDelay), true}, + {"pod clock badly ahead", now.Add(36 * time.Hour), true}, + // Unchanged behaviour on the in-step cases, so the clamp cannot be mistaken for + // "always settled". + {"quiet long enough", now.Add(-2 * untitledSettleDelay), true}, + {"still busy", now, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + m := newTitleModel(t, map[string]SessionMetadata{}, "s1") + m.sessions[0].UpdatedAt = tc.upd + if got := m.untitledSettled(now); got != tc.want { + t.Errorf("untitledSettled = %v, want %v (UpdatedAt %v from now)", + got, tc.want, tc.upd.Sub(now)) + } + }) + } +} From 908d70d24d91f86d0db0b6e9a179187451b6bb43 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Thu, 24 Sep 2026 07:18:25 -0400 Subject: [PATCH 09/10] fix: Do not let the counted set outlive the list it describes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit untitledCounted was maintained only inside the len(m.sessions) > 0 scoring guard, so it survived every period when the list was empty while still claiming to describe what was on screen. Two ways that showed: A session that left the list and returned with no non-empty scoring in between kept stale membership, so the returning row read as "already counted" and inherited the full 3m cap for its first title — measured at misses=9, the exact defect the fresh-row reset was added to remove. The harvests landing while a list is empty are the ordinary ones: Init harvests before its session fetch returns, backing out to the picker clears m.sessions, and the picker harvests on arrival. And backToPodsPane reset the counter but not the set, carrying the previous pod's unnamed ids into the next connection. A shared id — `default` is the bucket every pod's denial events aggregate into, and a redeployed agent can reuse one — then lost its fresh-row reset and counted a miss it had not earned (measured: 10s instead of 5s). Rebuild the set on every harvest, outside the scoring guard. The guard decides whether there is evidence to score; the set records what was on screen when last asked. An empty list has no unnamed rows, so rebuilding empties the set and a returning row is new again. Scoring stays inside the guard, so an unscoreable harvest is still neither progress nor a miss. backToPodsPane clears the set too. The rebuild already closes the hole on the first harvest after the list empties, so that assignment is not what fixes it — it is there because discarding what described the pod being left is that function's job. TestBackToPodsPane_ResetsTheBackoff asserted only untitledMisses and passed while the defect was live; it now asserts the set as well. Two new tests cover the paths nothing reached: a return with no intervening non-empty scoring, and a pod switch onto a shared session id. Three mutations, each caught by the test written for it. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/cmd/abctl/tui/app.go | 48 ++++++---- .../cmd/abctl/tui/sessions_title_test.go | 87 +++++++++++++++++++ 2 files changed, 120 insertions(+), 15 deletions(-) diff --git a/authbridge/cmd/abctl/tui/app.go b/authbridge/cmd/abctl/tui/app.go index 416b14520..6cbd21e3d 100644 --- a/authbridge/cmd/abctl/tui/app.go +++ b/authbridge/cmd/abctl/tui/app.go @@ -934,6 +934,13 @@ func (m *model) backToPodsPane() { // waits the 3m cap before its first scan instead of untitledSettleDelay — measured, not // supposed. m.untitledMisses = 0 + // AND THE SET THAT PRICES IT. Leaving it behind carried the previous pod's unnamed ids into + // the next connection, where a shared id — the `default` bucket every pod has, or a redeployed + // agent reusing one — read as "already counted" and lost its fresh-row reset, counting a miss + // it had not earned. The scoring rebuild above also clears this on the first harvest after the + // list empties, so this assignment is not what closes the hole; it is here because this + // function's job is to discard what described the pod being left, and the set describes it. + m.untitledCounted = nil m.previousPane = paneNone // Same reason: a return pane recorded against the pod being left would send // the next `P`-then-esc back into a pane belonging to the previous connection. @@ -1224,22 +1231,33 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // session is the bug. Ordered before the miss count so a tick that both gains a new row and // fails to name anything resets rather than incrementing: the new row has not been tried // yet, so there is no evidence against it to count. - if len(m.sessions) > 0 { - counted := make(map[string]bool, len(m.sessions)) - fresh := false - for _, sess := range m.sessions { - if m.sessionHasTitle(sess.ID) { - continue - } - counted[sess.ID] = true - if !m.untitledCounted[sess.ID] { - fresh = true - } + // + // THE SET IS REBUILT ON EVERY HARVEST, INCLUDING UNSCOREABLE ONES, and that is outside the + // len() > 0 guard for a reason the guard itself cannot serve. The guard decides whether + // there is EVIDENCE to score; the set records WHAT WAS ON SCREEN when it was last asked. + // Those are different questions, and keeping the set inside the guard answered the second + // one with a stale snapshot: an empty list left the previous list's ids in place, so a + // session that left and came back with no non-empty scoring in between was read as + // "already counted" and inherited the full 3m cap for its first title — measured at + // misses=9, which is the very defect the fresh-row reset exists to remove. An empty list + // has no unnamed rows, so rebuilding here correctly empties the set, and the returning row + // is new again. + counted := make(map[string]bool, len(m.sessions)) + fresh := false + for _, sess := range m.sessions { + if m.sessionHasTitle(sess.ID) { + continue } - // ASSIGNED BEFORE THE BRANCH, so every scoreable tick records what it judged. Doing it - // only on one arm would leave the set describing some earlier tick, and the next new - // row would be compared against a stale snapshot. - m.untitledCounted = counted + counted[sess.ID] = true + if !m.untitledCounted[sess.ID] { + fresh = true + } + } + // ASSIGNED BEFORE THE BRANCH, so every harvest records what it judged. Doing it only on one + // arm would leave the set describing some earlier tick, and the next new row would be + // compared against a stale snapshot. + m.untitledCounted = counted + if len(m.sessions) > 0 { switch { case fresh: m.untitledMisses = 0 diff --git a/authbridge/cmd/abctl/tui/sessions_title_test.go b/authbridge/cmd/abctl/tui/sessions_title_test.go index b3efe66b4..2c5ce56e9 100644 --- a/authbridge/cmd/abctl/tui/sessions_title_test.go +++ b/authbridge/cmd/abctl/tui/sessions_title_test.go @@ -1587,12 +1587,21 @@ func TestBackToPodsPane_ResetsTheBackoff(t *testing.T) { t.Fatalf("fixture did not reach the cap: %v", untitledBackoff(m.untitledMisses)) } + // The set that prices the counter is part of the same state. Asserting only the counter passed + // while the set still carried this pod's unnamed ids into the next connection. + m.untitledCounted = map[string]bool{"shared": true} + m.backToPodsPane() if m.untitledMisses != 0 { t.Errorf("untitledMisses = %d after backing out; the next pod inherits a %v delay", m.untitledMisses, untitledBackoff(m.untitledMisses)) } + if len(m.untitledCounted) != 0 { + t.Errorf("untitledCounted = %v after backing out; a session id shared with the next pod "+ + "(the `default` bucket, or a redeployed agent) reads as already counted and loses its "+ + "fresh-row reset", m.untitledCounted) + } } // TestUntitledSettled_BlankAndUnknownRows pins what counts as unnamed and as settled. @@ -1920,3 +1929,81 @@ func TestUntitledSettled_FutureUpdatedAtIsNotQuietForever(t *testing.T) { }) } } + +// TestUntitledMisses_ReturningSessionWithNoInterveningScoring is the path +// TestUntitledMisses_ReturningSessionCountsAsNew does not reach. +// +// That test scores a non-empty list while the session is away, which rebuilds the set and drops the +// absent id. This one never does: the list goes empty and the harvests that land while it is empty +// are unscoreable. Those are the ordinary ones — Init harvests before its session fetch returns, +// backing out to the picker sets m.sessions = nil, and the picker harvests on arrival. +// +// With the set maintained only inside the len(m.sessions) > 0 guard, the empty scoring left the +// previous list's ids in place, so the returning row read as "already counted" and inherited the +// full 3m cap for its first title — measured at misses=9. The set now rebuilds on every harvest: +// an empty list has no unnamed rows, so the set correctly empties and the row is new again. +func TestUntitledMisses_ReturningSessionWithNoInterveningScoring(t *testing.T) { + m := newTitleModel(t, map[string]SessionMetadata{}, "comesback") + m.sessions = []session.SessionSummary{{ID: "comesback", UpdatedAt: time.Now().Add(-time.Hour)}} + empty := map[string]SessionMetadata{} + + for i := 0; i < 9; i++ { + m.Update(harvestedMsg{meta: empty}) + } + if got := untitledBackoff(m.untitledMisses); got != untitledBackoffCap { + t.Fatalf("fixture did not reach the cap: misses=%d backoff=%v", m.untitledMisses, got) + } + + // Gone, and the only harvest landing while it is away has no rows to score. + m.sessions = nil + m.Update(harvestedMsg{meta: empty}) + + // THE COUNTER MUST NOT HAVE MOVED. An unscoreable harvest is neither progress nor a miss — + // pinned here too, because rebuilding the set outside the scoring guard must not be mistaken + // for scoring outside it. + if m.untitledMisses == 0 { + t.Errorf("an unscoreable harvest reset untitledMisses; the set rebuild leaked into scoring") + } + + // Back again, still unnamed and settled. + m.sessions = []session.SessionSummary{{ID: "comesback", UpdatedAt: time.Now().Add(-6 * time.Second)}} + m.Update(harvestedMsg{meta: empty}) + + if m.untitledMisses != 0 { + t.Errorf("untitledMisses = %d after the session returned with no non-empty scoring in "+ + "between; its first title waits %v", m.untitledMisses, untitledBackoff(m.untitledMisses)) + } +} + +// TestBackToPodsPane_ClearsTheCountedSet covers the pod switch end to end, through the real +// harvestedMsg handler rather than by assigning the set. +// +// Two pods can hold a session with the same id: `default` is the bucket every pod's denial events +// aggregate into, and a redeployed agent can reuse one. Carrying the previous pod's set across the +// switch made that id read as already counted, so the new pod's first harvest counted a miss the +// row had not earned instead of resetting on arrival. +func TestBackToPodsPane_ClearsTheCountedSet(t *testing.T) { + m := newTitleModel(t, map[string]SessionMetadata{}, "default") + m.parentCtx, m.ctx = context.Background(), context.Background() + m.cancel = func() {} + m.sessions = []session.SessionSummary{{ID: "default", UpdatedAt: time.Now().Add(-time.Hour)}} + empty := map[string]SessionMetadata{} + + for i := 0; i < 9; i++ { + m.Update(harvestedMsg{meta: empty}) + } + if got := untitledBackoff(m.untitledMisses); got != untitledBackoffCap { + t.Fatalf("fixture did not reach the cap: misses=%d backoff=%v", m.untitledMisses, got) + } + + m.backToPodsPane() + + // A different pod, whose list also holds a `default` session — unnamed, settled. + m.sessions = []session.SessionSummary{{ID: "default", UpdatedAt: time.Now().Add(-6 * time.Second)}} + m.Update(harvestedMsg{meta: empty}) + + if m.untitledMisses != 0 { + t.Errorf("untitledMisses = %d on the new pod's first scoring of a shared session id; "+ + "the previous pod's set suppressed the fresh-row reset", m.untitledMisses) + } +} From 4c6bf71241f3a122d5495e479a4462fe88261b30 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Thu, 24 Sep 2026 07:45:22 -0400 Subject: [PATCH 10/10] fix: Ask the harvest gate about a new row, not just the scoring The arrival reset added earlier could not help the row that caused it. The reset lives in the harvestedMsg handler, which runs when a harvest FINISHES; the backoff gate on the refreshTickMsg path is what decides whether one STARTS, and it read untitledMisses raw. So an unnameable row at untitledBackoffCap still made a brand-new settled session wait 3m for its first title, and the reset took effect only afterwards, for the next new row. Measured on the real Update loop: misses=11, backoff=3m0s, and a settled brand-new unnamed row does not fire a scan. The gate now prices a row the backoff was never earned against at untitledBackoff(0) -- the same floor any row's first attempt gets, so the arrival forgives the accumulated penalty without skipping the settle delay that makes this poll affordable. untitledFresh answers that question from untitledCounted, and the scoring's own walk moves into countUntitled so the gate's question and the scoring's bookkeeping cannot drift apart. Three tests drive refreshTickMsg through Update and assert on whether a harvest started: the fresh row harvests despite another row's backoff, the unnameable row still backs off at the gate, and a fresh row that is still receiving events still waits to settle. Four mutations, each caught by the test written for it. The existing arrival-reset tests assert untitledMisses and passed while this was live, which is how it survived three review rounds; that test now carries a comment saying the counter is not the user-visible behaviour and pointing at the gate test. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/cmd/abctl/tui/app.go | 38 ++++-- authbridge/cmd/abctl/tui/session_metadata.go | 54 +++++++++ .../cmd/abctl/tui/sessions_title_test.go | 113 ++++++++++++++++++ 3 files changed, 193 insertions(+), 12 deletions(-) diff --git a/authbridge/cmd/abctl/tui/app.go b/authbridge/cmd/abctl/tui/app.go index 6cbd21e3d..387c5b1c4 100644 --- a/authbridge/cmd/abctl/tui/app.go +++ b/authbridge/cmd/abctl/tui/app.go @@ -1242,17 +1242,12 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // misses=9, which is the very defect the fresh-row reset exists to remove. An empty list // has no unnamed rows, so rebuilding here correctly empties the set, and the returning row // is new again. - counted := make(map[string]bool, len(m.sessions)) - fresh := false - for _, sess := range m.sessions { - if m.sessionHasTitle(sess.ID) { - continue - } - counted[sess.ID] = true - if !m.untitledCounted[sess.ID] { - fresh = true - } - } + // THROUGH countUntitled's shared walk, so the gate's untitledFresh and this scoring judge + // "unnamed and not yet counted" by one definition. The gate forgives the backoff on that + // answer; this records it. If the two ever disagreed, an arrival would be forgiven and + // never recorded (harvesting every tick forever) or recorded without being forgiven (the + // defect this PR is about). + counted, fresh := m.countUntitled() // ASSIGNED BEFORE THE BRANCH, so every harvest records what it judged. Doing it only on one // arm would leave the set describing some earlier tick, and the next new row would be // compared against a stale snapshot. @@ -1378,6 +1373,16 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // row re-walks the transcript tree every untitledSettleDelay for a title that is not // coming. See untitledMisses. // + // EXCEPT FOR A ROW THE BACKOFF WAS NEVER EARNED AGAINST, which is what untitledFresh asks + // and what the scoring reset alone could not deliver. The reset in the harvestedMsg + // handler only runs after a harvest COMPLETES, so on the tick where a new session first + // appears the counter still holds the previous rows' penalty — and the gate is what + // decides whether that harvest ever starts. An unnameable row at the cap therefore made a + // brand-new settled session wait 3m for its first title, measured on the real Update loop, + // which is the very bug the reset was added to fix: the reset fires only on the harvest + // after the one the new row needed. Asking here closes it, because this is the only place + // the decision is actually made. + // // STARTED HERE, NOT RETURNED FROM HERE. This pane's tick must still reach the session // fetch at the bottom of this branch, so the harvest is batched into that return rather // than short-circuiting it — returning early instead would trade the titles for the @@ -1387,8 +1392,17 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // answered about the same instant rather than two readings of the clock a few // microseconds apart. now := time.Now() + // PRICED AT THE FLOOR FOR A FRESH ROW. untitledSettleDelay rather than zero, so a new + // session still waits for its transcript to settle — the arrival is a reason to forgive + // the accumulated penalty, not a reason to skip the settle test that makes this poll + // affordable. untitledBackoff(0) IS that floor, so this is the same number the first + // attempt at any row gets. + wait := untitledBackoff(m.untitledMisses) + if m.untitledFresh() { + wait = untitledBackoff(0) + } if m.pane == paneSessions && m.harvest != nil && !m.harvesting && - now.Sub(m.lastHarvest) >= untitledBackoff(m.untitledMisses) && + now.Sub(m.lastHarvest) >= wait && m.untitledSettled(now) { m.harvesting = true m.lastHarvest = now diff --git a/authbridge/cmd/abctl/tui/session_metadata.go b/authbridge/cmd/abctl/tui/session_metadata.go index 8702ed693..7756ec1ff 100644 --- a/authbridge/cmd/abctl/tui/session_metadata.go +++ b/authbridge/cmd/abctl/tui/session_metadata.go @@ -235,6 +235,60 @@ func (m *model) untitledSettled(now time.Time) bool { return false } +// untitledFresh reports whether some unnamed row on screen was NOT counted against the backoff +// yet — a session that appeared since the last harvest was scored. +// +// ASKED BY THE GATE, WHICH IS THE POINT. untitledMisses is reset for the same reason in the +// harvestedMsg handler, but that reset lands one harvest too late to help the row that caused it: +// the handler runs when a harvest FINISHES, and the gate decides whether one STARTS. So a row +// arriving while an unnameable row held the counter at untitledBackoffCap waited out a 3m penalty +// it had no part in earning, and the reset only took effect afterwards — for the next new row. +// Reading the set here means the arrival is priced on the tick it arrives. +// +// COSTS ONE MAP LOOKUP PER UNNAMED ROW PER TICK, and only while the sessions pane is open. The +// steady state — every row titled — exits on sessionHasTitle without touching the set at all, +// and the loop is over one pod's live sessions. It is the same walk untitledSettled does and the +// same walk the scoring does; see countUntitled, which the scoring shares with this. +// +// DOES NOT MUTATE THE SET. The gate asks a question; the harvest's scoring is what records the +// answer. Updating membership here would consume the freshness before the harvest it authorised +// could be judged, so an arrival would forgive the backoff and then, if the harvest named +// nothing, be counted as a miss for a row that had never been tried — which is exactly the +// ordering the scoring's `fresh` arm exists to avoid. +func (m *model) untitledFresh() bool { + for _, sess := range m.sessions { + if m.sessionHasTitle(sess.ID) { + continue + } + if !m.untitledCounted[sess.ID] { + return true + } + } + return false +} + +// countUntitled returns the set of on-screen rows that have no title, and whether any of them is +// one untitledCounted has not seen. +// +// ONE WALK SHARED BY THE GATE'S QUESTION AND THE SCORING'S BOOKKEEPING, because they must agree +// about what "unnamed and not yet counted" means. untitledFresh answers the gate from the same +// predicate this builds the set from; a second inline copy of the loop is how the two would drift +// into disagreeing, which would show up as either a forgiven backoff that never gets recorded or a +// recorded row that never got forgiven. +func (m *model) countUntitled() (counted map[string]bool, fresh bool) { + counted = make(map[string]bool, len(m.sessions)) + for _, sess := range m.sessions { + if m.sessionHasTitle(sess.ID) { + continue + } + counted[sess.ID] = true + if !m.untitledCounted[sess.ID] { + fresh = true + } + } + return counted, fresh +} + // sessionHasTitle reports whether this session renders a title, as the TITLE cell would judge it. // // THROUGH sessionTitle, not the raw map, so this predicate and the cell can never disagree about diff --git a/authbridge/cmd/abctl/tui/sessions_title_test.go b/authbridge/cmd/abctl/tui/sessions_title_test.go index 2c5ce56e9..534be3827 100644 --- a/authbridge/cmd/abctl/tui/sessions_title_test.go +++ b/authbridge/cmd/abctl/tui/sessions_title_test.go @@ -1783,6 +1783,13 @@ func TestPicker_InitHarvestCountsAgainstTheVisit(t *testing.T) { // // DRIVES THE REAL HANDLER, not the fields. The reset has to happen where the misses are counted, so // a test that set untitledMisses by hand would pass against a fix placed anywhere at all. +// +// ASSERTS THE COUNTER, WHICH IS NOT THE USER-VISIBLE BEHAVIOUR — and reading it as though it were is +// how a live defect sat behind a passing test. The scoring reset this checks runs after a harvest +// FINISHES, so it cannot help the row that triggered it; whether that row's harvest ever STARTS is +// decided by the backoff gate on the refreshTickMsg path. See +// TestRefreshTick_FreshRowHarvestsDespiteAnotherRowsBackoff, which measures that, and keep the two +// together: this one pins the bookkeeping, that one pins the timing. func TestUntitledMisses_NewSessionClearsAnotherRowsPenalty(t *testing.T) { // One row that no harvest will ever name. m := newTitleModel(t, map[string]SessionMetadata{}, "unnameable") @@ -2007,3 +2014,109 @@ func TestBackToPodsPane_ClearsTheCountedSet(t *testing.T) { "the previous pod's set suppressed the fresh-row reset", m.untitledMisses) } } + +// TestRefreshTick_FreshRowHarvestsDespiteAnotherRowsBackoff is the arrival reset measured where an +// operator feels it: whether a harvest actually STARTS. +// +// THE COUNTER TESTS ABOVE CANNOT CATCH THIS, which is why this one exists. They hand the handler a +// harvestedMsg and assert untitledMisses == 0 — true, and irrelevant to the row that needed it. The +// scoring reset runs when a harvest FINISHES; the backoff gate on the refreshTickMsg path decides +// whether one BEGINS. So with the reset in place and the gate reading the raw counter, an unnameable +// row at untitledBackoffCap still made a brand-new settled session wait 3m for its first title, and +// every arrival-reset test passed the whole time. The gate is the only place this is observable. +// +// DRIVES refreshTickMsg THROUGH Update, not the gate expression, so it fails if the freshness check +// is placed anywhere that is not the decision itself. +func TestRefreshTick_FreshRowHarvestsDespiteAnotherRowsBackoff(t *testing.T) { + m := newTitleModel(t, map[string]SessionMetadata{}, "unnameable") + empty := map[string]SessionMetadata{} + m.harvest = func() (map[string]SessionMetadata, error) { return empty, nil } + + // Drive the unnameable row past the cap through the real handler. + m.sessions = []session.SessionSummary{{ID: "unnameable", UpdatedAt: time.Now().Add(-time.Minute)}} + for i := 0; i < 10; i++ { + m.Update(harvestedMsg{meta: empty}) + } + if got := untitledBackoff(m.untitledMisses); got != untitledBackoffCap { + t.Fatalf("fixture did not reach the cap: misses=%d backoff=%v", m.untitledMisses, got) + } + + // A new session appears and settles. Ten seconds since the last harvest: past + // untitledSettleDelay, nowhere near the 3m the other row earned. + m.harvesting = false + m.lastHarvest = time.Now().Add(-10 * time.Second) + m.sessions = append(m.sessions, + session.SessionSummary{ID: "brandnew", UpdatedAt: time.Now().Add(-untitledSettleDelay * 2)}) + + m.Update(refreshTickMsg{}) + + // m.harvesting is the gate's own record that it fired, and the one the batched command is + // guarded by — asserting the returned tea.Cmd is non-nil would pass on the refresh tick alone. + if !m.harvesting { + t.Errorf("no harvest started for a brand-new settled session while another row held the "+ + "backoff at %v; its first title waits for a penalty it did not earn", + untitledBackoff(m.untitledMisses)) + } +} + +// TestRefreshTick_UnnameableRowStillBacksOffAtTheGate is the other side of the gate check. +// +// Forgiving the backoff for a row the set has not seen must not forgive it for the row that earned +// it. If untitledFresh answered yes whenever any unnamed row was on screen — or if the gate used the +// floor unconditionally — the transcript-tree walk every untitledSettleDelay would be back, which is +// the loop untitledMisses exists to break. +func TestRefreshTick_UnnameableRowStillBacksOffAtTheGate(t *testing.T) { + m := newTitleModel(t, map[string]SessionMetadata{}, "unnameable") + empty := map[string]SessionMetadata{} + m.harvest = func() (map[string]SessionMetadata, error) { return empty, nil } + m.sessions = []session.SessionSummary{{ID: "unnameable", UpdatedAt: time.Now().Add(-time.Minute)}} + + // Six harvests: five misses, so the earned wait is well past untitledSettleDelay. + for i := 0; i < 6; i++ { + m.Update(harvestedMsg{meta: empty}) + } + earned := untitledBackoff(m.untitledMisses) + if earned <= untitledSettleDelay { + t.Fatalf("fixture did not back off: misses=%d backoff=%v", m.untitledMisses, earned) + } + + // Past the settle floor, inside the earned backoff. Nothing new on screen. + m.harvesting = false + m.lastHarvest = time.Now().Add(-untitledSettleDelay * 2) + + m.Update(refreshTickMsg{}) + + if m.harvesting { + t.Errorf("harvested %v after the last one despite an earned backoff of %v; the same "+ + "unnameable row re-walks the transcript tree", untitledSettleDelay*2, earned) + } +} + +// TestRefreshTick_FreshRowStillWaitsToSettle keeps the arrival reset from eating the settle delay. +// +// A new row forgives the accumulated PENALTY, not the wait that makes this poll affordable: its +// transcript is being appended to, and the title tiers read the last message, so harvesting the +// instant a session appears reads a file the agent is still writing. The gate prices a fresh row at +// untitledBackoff(0), which IS untitledSettleDelay — not at zero. +func TestRefreshTick_FreshRowStillWaitsToSettle(t *testing.T) { + m := newTitleModel(t, map[string]SessionMetadata{}, "unnameable") + empty := map[string]SessionMetadata{} + m.harvest = func() (map[string]SessionMetadata, error) { return empty, nil } + m.sessions = []session.SessionSummary{{ID: "unnameable", UpdatedAt: time.Now().Add(-time.Minute)}} + for i := 0; i < 6; i++ { + m.Update(harvestedMsg{meta: empty}) + } + + // Long past any backoff, so the settle test is the only thing that can refuse. The new row is + // BUSY — updated a moment ago, transcript still being written. + m.harvesting = false + m.lastHarvest = time.Now().Add(-time.Hour) + m.sessions = []session.SessionSummary{{ID: "brandnew", UpdatedAt: time.Now()}} + + m.Update(refreshTickMsg{}) + + if m.harvesting { + t.Errorf("harvested a brand-new session that is still receiving events; the settle delay " + + "exists because the title tiers read a transcript the agent has not finished writing") + } +}