From eadd659f4b20204c061c261c25b6efbd9a34a25a Mon Sep 17 00:00:00 2001 From: Jorge Cajas Date: Fri, 31 Jul 2026 05:18:25 +0800 Subject: [PATCH 1/3] fix(#6047): wizard names outstanding stages at Done, labels RSS as peak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two completion-honesty defects in the index wizard TUI: 1. The wizard printed "Done" while group-algo/links were still running post-extraction (communities/pagerank/centrality overlay, cross-repo links), with no signal that anything was outstanding -- unlike `grafel status`, which already carries this caveat via its annotation/stage_gate lines. The wizard still doesn't block (the graph IS queryable at Done), but now queries the daemon's status once more at completion and appends a caveat naming which stage(s) are still running/pending, reusing grafel status's own wording. 2. The "0.9 GB" memory readout was the engine's live instantaneous RSS at wizard exit, positioned next to "Done · 15m55s" so it read as the run's total -- understating the measured peak by roughly 4x. rssMB now tracks the running max across every poll instead of the latest sample, and the readout is explicitly labelled "peak". Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0111KEDUVWEWm9G5RRnJqXft --- internal/cli/wizard_outstanding.go | 107 +++++++++++++++++ internal/cli/wizard_outstanding_test.go | 131 +++++++++++++++++++++ internal/cli/wizard_split_progress_test.go | 4 +- internal/cli/wizard_tui_run.go | 25 ++-- internal/cli/wiztui/indexview.go | 79 ++++++++++--- internal/cli/wiztui/indexview_test.go | 31 +++++ internal/cli/wiztui/model.go | 27 ++++- internal/cli/wiztui/model_test.go | 88 ++++++++++++++ internal/cli/wiztui/view_test.go | 17 +++ 9 files changed, 481 insertions(+), 28 deletions(-) create mode 100644 internal/cli/wizard_outstanding.go create mode 100644 internal/cli/wizard_outstanding_test.go diff --git a/internal/cli/wizard_outstanding.go b/internal/cli/wizard_outstanding.go new file mode 100644 index 000000000..268c53238 --- /dev/null +++ b/internal/cli/wizard_outstanding.go @@ -0,0 +1,107 @@ +package cli + +// wizard_outstanding.go — the wizard index screen's completion-honesty caveat +// (#6047). +// +// THE BUG THIS FIXES: in split mode the wizard's "Done" fires the instant the +// engine drains+acks our rebuild request (see wizard_split_progress.go's +// awaitSplitCompletion) — i.e. once extraction + the cross-repo link merge +// have written graph.fb. The graph genuinely IS queryable at that point (every +// structural MCP tool works), but the community/pagerank/centrality overlay +// (group-algo) and, in some orderings, the cross-repo link pass itself are +// stage-gated to run AFTER that ack, in the background, via the SAME +// stage-gate `grafel status` already reports through its annotation/ +// stage_gate lines (see internal/cli/status_annotation.go and +// status_daemon_detail.go). A user or agent that starts querying the instant +// the wizard exits gets an overlay that is silently not current, with no +// signal from the wizard that anything is outstanding — the same defect class +// `grafel status` already had to fix for itself. +// +// THE FIX: do NOT block wizard completion on the gate releasing (that would +// turn a ~16-minute run into a ~26-minute one for a large group, for no user +// benefit — the graph is already queryable). Instead, query the daemon's +// status ONE more time the instant the terminal outcome is available and, if +// group-algo or links is still running/pending for OUR group, attach a caveat +// naming it — in the SAME vocabulary status_annotation.go's +// "communities/pagerank/centrality overlay not yet current; the graph itself +// is queryable" line already uses — to wiztui.IndexOutcome.Outstanding. Silent +// (empty string) when nothing is outstanding, which is the common case (a +// small group, or an already-warm overlay). + +import ( + "fmt" + "slices" + "strings" + + "github.com/cajasmota/grafel/internal/daemon/client" + "github.com/cajasmota/grafel/internal/daemon/proto" +) + +// outstandingOverlayCaveat is the exact phrase status_annotation.go's +// printAnnotationStatus already prints for a running/pending group-algo pass — +// reused verbatim here so the wizard and `grafel status` describe the same +// state in the same words. +const outstandingOverlayCaveat = "communities/pagerank/centrality overlay not yet current; the graph itself is queryable" + +// outstandingCaveat derives the wizard's completion caveat naming which +// post-extraction stage(s) — group-algo, links, or both — are still running +// or pending for group, purely from a proto.StatusReply snapshot. Pure and +// side-effect-free so it is unit-testable without a daemon. +// +// A stage counts as OUTSTANDING for group if either the coarse +// running/pending lists name it (GroupAlgoRunning/PendingAlgo/PendingLinks — +// the same fields printAnnotationStatus and printDaemonDetail's scheduler +// line read) or the fine-grained stage gate (StageGateHolder/ +// StageGateDeferred) names "group-algo:" / "links:" — the exact +// holder/deferred naming scheme documented on +// internal/daemon/sched/scheduler.go's StageHolder/StageDeferred fields. +// Checking both catches every mode: split-mode's status snapshot populates +// the coarse lists (Service.Status' snap.PendingAlgo / snap.GroupAlgoRunning +// branch), monolith's populates the stage gate directly (the `g.Holder` / +// `g.Deferred` branch) — see internal/daemon/service.go. +// +// Returns "" when neither stage is outstanding for group — the caller (see +// attachOutstanding) leaves wiztui.IndexOutcome.Outstanding "" in that case, +// and indexView renders nothing extra (a caveat, not decoration). +func outstandingCaveat(st proto.StatusReply, group string) string { + algoRunning := slices.Contains(st.GroupAlgoRunning, group) || st.StageGateHolder == "group-algo:"+group + algoPending := slices.Contains(st.PendingAlgo, group) || slices.Contains(st.StageGateDeferred, "group-algo:"+group) + linksRunning := st.StageGateHolder == "links:"+group + linksPending := slices.Contains(st.PendingLinks, group) || slices.Contains(st.StageGateDeferred, "links:"+group) + + var stages []string + switch { + case algoRunning: + stages = append(stages, "group-algo") + case algoPending: + stages = append(stages, "group-algo (pending)") + } + switch { + case linksRunning: + stages = append(stages, "links") + case linksPending: + stages = append(stages, "links (pending)") + } + if len(stages) == 0 { + return "" + } + return fmt.Sprintf("still running: %s (%s)", strings.Join(stages, ", "), outstandingOverlayCaveat) +} + +// attachOutstanding queries the daemon's current status and returns the +// completion caveat for group (see outstandingCaveat), or "" on ANY failure — +// a dead/unreachable client, an RPC error, or nothing outstanding. This is a +// best-effort, non-blocking honesty check on an already-successful +// completion, never a reason to fail or delay it: a status query that errors +// degrades to "no caveat shown" exactly like an absent rssMB reading degrades +// to "no readout shown" (see wizard_metrics.go's engineMetricsFuncWith). +func attachOutstanding(c *client.Client, group string) string { + if c == nil { + return "" + } + st, err := c.Status() + if err != nil { + return "" + } + return outstandingCaveat(st, group) +} diff --git a/internal/cli/wizard_outstanding_test.go b/internal/cli/wizard_outstanding_test.go new file mode 100644 index 000000000..06a2ca74b --- /dev/null +++ b/internal/cli/wizard_outstanding_test.go @@ -0,0 +1,131 @@ +package cli + +import ( + "strings" + "testing" + + "github.com/cajasmota/grafel/internal/daemon/proto" +) + +// TestOutstandingCaveat_NothingOutstanding_ReturnsEmpty pins direction 2 for +// #6047: a status snapshot where nothing names our group (an already-warm +// overlay, or a small group that finished before the wizard even asked) must +// produce NO caveat. This must be checked against a StatusReply that DOES +// name a DIFFERENT group in every field, proving the emptiness is really +// group-scoped filtering and not just "always returns ”". +func TestOutstandingCaveat_NothingOutstanding_ReturnsEmpty(t *testing.T) { + st := proto.StatusReply{ + GroupAlgoRunning: []string{"other-group"}, + PendingAlgo: []string{"other-group"}, + PendingLinks: []string{"other-group"}, + StageGateHolder: "group-algo:other-group", + StageGateDeferred: []string{"links:other-group"}, + } + got := outstandingCaveat(st, "mygroup") + if got != "" { + t.Errorf("outstandingCaveat = %q, want empty (nothing names mygroup)", got) + } +} + +// TestOutstandingCaveat_TrulyIdle_ReturnsEmpty pins the plain idle case: an +// entirely empty StatusReply. +func TestOutstandingCaveat_TrulyIdle_ReturnsEmpty(t *testing.T) { + got := outstandingCaveat(proto.StatusReply{}, "mygroup") + if got != "" { + t.Errorf("outstandingCaveat = %q, want empty on a fully idle status", got) + } +} + +// TestOutstandingCaveat_GroupAlgoRunning_ReturnsCaveat pins direction 1 via +// the coarse GroupAlgoRunning list (split-mode's status snapshot source — +// see internal/daemon/service.go's snap.GroupAlgoRunning branch). +func TestOutstandingCaveat_GroupAlgoRunning_ReturnsCaveat(t *testing.T) { + st := proto.StatusReply{GroupAlgoRunning: []string{"mygroup"}} + got := outstandingCaveat(st, "mygroup") + if got == "" { + t.Fatal("outstandingCaveat = \"\", want a non-empty caveat") + } + if !strings.Contains(got, "group-algo") { + t.Errorf("caveat missing stage name %q: %q", "group-algo", got) + } + if !strings.Contains(got, outstandingOverlayCaveat) { + t.Errorf("caveat must reuse `grafel status`'s exact overlay wording:\ngot: %q\nwant substring: %q", got, outstandingOverlayCaveat) + } +} + +// TestOutstandingCaveat_PendingAlgo_ReturnsCaveat pins the PENDING (not yet +// running, debounce-armed) group-algo case, phrased distinctly from RUNNING. +func TestOutstandingCaveat_PendingAlgo_ReturnsCaveat(t *testing.T) { + st := proto.StatusReply{PendingAlgo: []string{"mygroup"}} + got := outstandingCaveat(st, "mygroup") + if !strings.Contains(got, "group-algo (pending)") { + t.Errorf("caveat = %q, want it to name group-algo as pending", got) + } +} + +// TestOutstandingCaveat_StageGateHolder_GroupAlgo_ReturnsCaveat pins the +// fine-grained stage-gate holder naming scheme ("group-algo:", the +// exact string internal/daemon/sched/scheduler.go's group-algo pass acquires +// — see stageAcquire's "group-algo:" + group) — the source monolith-mode's +// status snapshot uses (the `g.Holder` branch in service.go). +func TestOutstandingCaveat_StageGateHolder_GroupAlgo_ReturnsCaveat(t *testing.T) { + st := proto.StatusReply{StageGateHolder: "group-algo:mygroup"} + got := outstandingCaveat(st, "mygroup") + if !strings.Contains(got, "group-algo") { + t.Errorf("caveat = %q, want it to name group-algo", got) + } +} + +// TestOutstandingCaveat_StageGateHolder_Links_ReturnsCaveat pins the "links" +// holder case — the cross-repo link pass that produces cross-repo edges/flows +// (the second stage #6047 names, distinct from group-algo). +func TestOutstandingCaveat_StageGateHolder_Links_ReturnsCaveat(t *testing.T) { + st := proto.StatusReply{StageGateHolder: "links:mygroup"} + got := outstandingCaveat(st, "mygroup") + if !strings.Contains(got, "links") { + t.Errorf("caveat = %q, want it to name links", got) + } +} + +// TestOutstandingCaveat_StageGateDeferred_Links_ReturnsCaveat pins the +// deferred (turned-away, waiting its turn behind the gate) links case. +func TestOutstandingCaveat_StageGateDeferred_Links_ReturnsCaveat(t *testing.T) { + st := proto.StatusReply{StageGateDeferred: []string{"links:mygroup"}} + got := outstandingCaveat(st, "mygroup") + if !strings.Contains(got, "links (pending)") { + t.Errorf("caveat = %q, want it to name links as pending", got) + } +} + +// TestOutstandingCaveat_PendingLinks_ReturnsCaveat pins the coarse +// PendingLinks list. +func TestOutstandingCaveat_PendingLinks_ReturnsCaveat(t *testing.T) { + st := proto.StatusReply{PendingLinks: []string{"mygroup"}} + got := outstandingCaveat(st, "mygroup") + if !strings.Contains(got, "links (pending)") { + t.Errorf("caveat = %q, want it to name links as pending", got) + } +} + +// TestOutstandingCaveat_BothStagesOutstanding_NamesBoth pins a real-world +// snapshot shaped like the issue's own reproduction: links holding the gate +// while group-algo is deferred behind it — both must be named. +func TestOutstandingCaveat_BothStagesOutstanding_NamesBoth(t *testing.T) { + st := proto.StatusReply{ + StageGateHolder: "links:mygroup", + StageGateDeferred: []string{"group-algo:mygroup"}, + } + got := outstandingCaveat(st, "mygroup") + if !strings.Contains(got, "group-algo (pending)") || !strings.Contains(got, "links") { + t.Errorf("caveat = %q, want both group-algo (pending) and links named", got) + } +} + +// TestAttachOutstanding_NilClient_ReturnsEmpty: attachOutstanding must never +// panic or block on a nil client (e.g. --no-index / daemon-down paths that +// never dial) — it degrades to "no caveat", same as an absent RSS reading. +func TestAttachOutstanding_NilClient_ReturnsEmpty(t *testing.T) { + if got := attachOutstanding(nil, "mygroup"); got != "" { + t.Errorf("attachOutstanding(nil, ...) = %q, want empty", got) + } +} diff --git a/internal/cli/wizard_split_progress_test.go b/internal/cli/wizard_split_progress_test.go index b36dee2c2..88693941f 100644 --- a/internal/cli/wizard_split_progress_test.go +++ b/internal/cli/wizard_split_progress_test.go @@ -242,7 +242,7 @@ func TestSplit_OutcomeCarriesClassifiedStats(t *testing.T) { if o.err != nil { t.Fatalf("unexpected error: %v", o.err) } - oc := toIndexOutcome(o, wiztui.InstallSummary{}) + oc := toIndexOutcome(o, wiztui.InstallSummary{}, nil, "") if oc.Entities != E || oc.Rels != R { t.Fatalf("IndexOutcome stats = (%d,%d); want (%d,%d)", oc.Entities, oc.Rels, E, R) } @@ -847,7 +847,7 @@ func TestRunSplitIndexCore_ThreadsPerRepoStatsIntoOutcome(t *testing.T) { t.Fatalf("len(o.repoStats) = %d, want 3", len(o.repoStats)) } - oc := toIndexOutcome(o, wiztui.InstallSummary{}) + oc := toIndexOutcome(o, wiztui.InstallSummary{}, nil, "") if len(oc.RepoStats) != 3 { t.Fatalf("len(IndexOutcome.RepoStats) = %d, want 3", len(oc.RepoStats)) } diff --git a/internal/cli/wizard_tui_run.go b/internal/cli/wizard_tui_run.go index 40f27f086..7b6173621 100644 --- a/internal/cli/wizard_tui_run.go +++ b/internal/cli/wizard_tui_run.go @@ -404,7 +404,7 @@ func streamIndexWithSummary(evCh chan<- progress.Event, outCh chan<- wiztui.Inde if !perRepoRows { o.repoStats = nil // monorepo: suppress the aggregate-as-repo-row overlay } - outCh <- toIndexOutcome(o, summary) + outCh <- toIndexOutcome(o, summary, c, group) return } @@ -421,7 +421,7 @@ func streamIndexWithSummary(evCh chan<- progress.Event, outCh chan<- wiztui.Inde rpcCh := triggerRebuild(c, group, token) o := forwardBrokerToChannel(ctx, sseCh, rpcCh, evCh) cancel() - outCh <- toIndexOutcome(o, summary) + outCh <- toIndexOutcome(o, summary, c, group) return } } @@ -429,7 +429,7 @@ func streamIndexWithSummary(evCh chan<- progress.Event, outCh chan<- wiztui.Inde // No dashboard broker — just trigger the rebuild and wait for the outcome. rpcCh := triggerRebuild(c, group, token) o := <-rpcCh - outCh <- toIndexOutcome(o, summary) + outCh <- toIndexOutcome(o, summary, c, group) } // triggerRebuild fires the daemon Rebuild RPC for group on a goroutine and @@ -523,8 +523,12 @@ func jsonUnmarshalEvent(data string, e *progress.Event) bool { } // toIndexOutcome maps a rebuildOutcome to the TUI's terminal IndexOutcome, -// attaching the captured install summary so the Done screen renders it. -func toIndexOutcome(o rebuildOutcome, summary wiztui.InstallSummary) wiztui.IndexOutcome { +// attaching the captured install summary so the Done screen renders it. On a +// genuine success it also attaches the completion-honesty caveat (#6047) by +// querying the daemon's status ONE more time via c for group — best-effort, +// see attachOutstanding's doc; never attempted on an error outcome, since +// nothing was indexed to have an outstanding stage. +func toIndexOutcome(o rebuildOutcome, summary wiztui.InstallSummary, c *client.Client, group string) wiztui.IndexOutcome { if o.err != nil { return wiztui.IndexOutcome{Err: o.err, Install: summary} } @@ -533,11 +537,12 @@ func toIndexOutcome(o rebuildOutcome, summary wiztui.InstallSummary) wiztui.Inde elapsed = fmtDuration(time.Duration(o.elapsed * float64(time.Second))) } return wiztui.IndexOutcome{ - Entities: o.entities, - Rels: o.rels, - Elapsed: elapsed, - Install: summary, - RepoStats: toWiztuiRepoStats(o.repoStats), + Entities: o.entities, + Rels: o.rels, + Elapsed: elapsed, + Install: summary, + RepoStats: toWiztuiRepoStats(o.repoStats), + Outstanding: attachOutstanding(c, group), } } diff --git a/internal/cli/wiztui/indexview.go b/internal/cli/wiztui/indexview.go index 2c1af4a36..197e770ba 100644 --- a/internal/cli/wiztui/indexview.go +++ b/internal/cli/wiztui/indexview.go @@ -85,16 +85,45 @@ type indexView struct { // bgAnimDir is the current direction (+1 or -1) of the bgPct sweep. bgAnimDir float64 - // rssMB / cpuPct are the engine process's live CPU/RAM readout (wizard - // CPU/RAM readout — see internal/statusfile.File's RSSMB/CPUPct doc), - // polled periodically from the engine-liveness status-plane sidecar via - // Model's metricsFn (see model.go). Zero/absent means "unknown or not yet - // polled" and the readout is omitted entirely — never rendered as a - // misleading 0%/0.0 GB. rssMB is the must-have signal (it shows the - // multi-GB enrichment-phase peak); cpuPct is best-effort and independently - // omittable. + // rssMB / cpuPct are the engine process's CPU/RAM readout (wizard CPU/RAM + // readout — see internal/statusfile.File's RSSMB/CPUPct doc), polled + // periodically from the engine-liveness status-plane sidecar via Model's + // metricsFn (see model.go). Zero/absent means "unknown or not yet polled" + // and the readout is omitted entirely — never rendered as a misleading + // 0%/0.0 GB. + // + // rssMB is the RUNNING PEAK, not the latest instantaneous sample (#6047): + // Model's metricsMsg handler only ever raises it, never lowers it. A raw + // live reading taken at the moment the wizard exits is whatever the engine + // happened to be using at that instant — typically well past its own + // multi-GB enrichment-phase high-water mark, since RSS falls once + // enrichment finishes and buffers are released — and next to "Done · + // 15m55s" it reads as "this run used 0.9 GB" when the run's actual peak + // was several times that. Tracking the max of every poll and labelling it + // "peak" (see metricSuffix) reports the same honest in-process metric + // (memtrace's rss_bytes, which tracks `ps` within a few MB — this is a + // peak-vs-instant LABELLING fix, not a measurement fix) as what the run + // actually used, not a random late sample of it. cpuPct stays a live + // instantaneous reading (best-effort, independently omittable) — there is + // no equivalent "peak CPU%" complaint from #6047, and peak-CPU% is a much + // noisier, less meaningful number than peak RSS. rssMB int64 cpuPct float64 + + // outstanding names the post-extraction stage(s) (group-algo / links) for + // this group that were still running or pending the instant the wizard's + // terminal outcome landed, in the same vocabulary `grafel status`'s + // annotation/stage_gate lines use (#6047) — or "" when nothing was + // outstanding (the common case: a small group, or an already-warm + // overlay). The wizard deliberately does NOT block on this: the graph + // itself is queryable the instant extraction + the cross-repo link merge + // finish, and waiting for the community/pagerank/centrality overlay too + // would turn a ~16-minute run into a ~26-minute one for no user benefit. + // This is purely a completion-honesty caveat appended to doneSummary, set + // once from IndexOutcome.Outstanding on the terminal outcome — never + // updated afterward (the wizard has already exited its polling loops by + // then). + outstanding string } func newIndexView(group string, expectedRepos int) indexView { @@ -352,11 +381,20 @@ func (v indexView) renderRow(r Row, spinnerFrame string) string { return fmt.Sprintf("%s %s %s%s", glyph, name, phase, tail) } -// metricSuffix renders the live "CPU / RAM" readout that appears to the right -// of the overall progress bar's percentage — reassurance that a large-monorepo -// rebuild's multi-minute post-index enrichment phase (where the bar sits near -// 100% for a long stretch) is still doing real work, not stuck (motivation for -// this whole feature). +// metricSuffix renders the live "CPU / peak RAM" readout that appears to the +// right of the overall progress bar's percentage — reassurance that a +// large-monorepo rebuild's multi-minute post-index enrichment phase (where the +// bar sits near 100% for a long stretch) is still doing real work, not stuck +// (motivation for this whole feature). +// +// The RAM figure is explicitly labelled "peak" (#6047): v.rssMB is the max of +// every poll this run has seen (see indexView.rssMB's doc), not a live instant +// reading, and next to a frozen "Done · 15m55s" an unlabelled instant reads as +// the run's total when it is really whatever the engine happened to be using +// at the moment the wizard exited — often a fraction of the real high-water +// mark. cpuPct stays a live instantaneous reading, so it keeps its plain "CPU" +// label rather than "peak CPU" (no equivalent complaint, and a live number +// still reads correctly as a live number even while indexing is ongoing). // // Omits gracefully: an absent/zero rssMB (old status file predating this // field, engine metric read failed, or no poll has landed yet) returns "" and @@ -371,9 +409,9 @@ func (v indexView) metricSuffix() string { gb := float64(v.rssMB) / 1024.0 var text string if v.cpuPct > 0 { - text = fmt.Sprintf("%s CPU %.0f%% %s %.1f GB", g.MidDot, v.cpuPct, g.MidDot, gb) + text = fmt.Sprintf("%s CPU %.0f%% %s peak %.1f GB", g.MidDot, v.cpuPct, g.MidDot, gb) } else { - text = fmt.Sprintf("%s %.1f GB", g.MidDot, gb) + text = fmt.Sprintf("%s peak %.1f GB", g.MidDot, gb) } return " " + rowCountStyle.Render(text) } @@ -578,5 +616,16 @@ func (v indexView) doneSummary() string { b.WriteString(rowWarnStyle.Render(g.Warn + " " + w)) } + // Completion-honesty caveat (#6047): "Done" covers extraction + the + // cross-repo link merge — the graph itself is queryable — but the + // community/pagerank/centrality overlay (and, while links itself is the + // outstanding stage, cross-repo edges/flows) may still be computing in the + // background. Silent when nothing was outstanding at completion (the + // common case), never printed for a failed/daemon-down outcome. + if v.outstanding != "" { + b.WriteString("\n") + b.WriteString(rowWarnStyle.Render(g.Warn + " " + v.outstanding)) + } + return b.String() } diff --git a/internal/cli/wiztui/indexview_test.go b/internal/cli/wiztui/indexview_test.go index 8d70970b9..3c0d6428d 100644 --- a/internal/cli/wiztui/indexview_test.go +++ b/internal/cli/wiztui/indexview_test.go @@ -266,6 +266,37 @@ func TestIndexView_DoneSummary_Commafied(t *testing.T) { } } +// TestIndexView_DoneSummary_OutstandingCaveat_PresentAndAbsent is the pinning +// test for #6047's completion-honesty caveat: doneSummary must render the +// outstanding-stage line when v.outstanding is set (a fixture WITH an +// outstanding post-extraction stage), and must NOT render it — nor any stray +// "still running"/overlay text — when v.outstanding is empty (a fixture with +// none). Both directions are asserted against the SAME base view so neither +// branch is a fixture that can only ever prove one outcome. +func TestIndexView_DoneSummary_OutstandingCaveat_PresentAndAbsent(t *testing.T) { + base := newIndexView("grp", 1) + base.terminal = true + base.summaryEntities = 100 + base.summaryRels = 200 + + // Direction 1: nothing outstanding — the caveat must be absent entirely. + clean := base + clean.outstanding = "" + cleanOut := clean.doneSummary() + if strings.Contains(cleanOut, "still running") { + t.Errorf("doneSummary rendered a caveat with nothing outstanding:\n%s", cleanOut) + } + + // Direction 2: an outstanding stage — the exact caveat text must render. + caveat := "still running: group-algo (communities/pagerank/centrality overlay not yet current; the graph itself is queryable)" + dirty := base + dirty.outstanding = caveat + dirtyOut := dirty.doneSummary() + if !strings.Contains(dirtyOut, caveat) { + t.Errorf("doneSummary missing outstanding caveat:\ngot:\n%s\nwant substring:\n%s", dirtyOut, caveat) + } +} + // TestRenderRow_FilesCountCommafied asserts the per-row files-done/total and // entities-so-far counters render with thousands separators. func TestRenderRow_FilesCountCommafied(t *testing.T) { diff --git a/internal/cli/wiztui/model.go b/internal/cli/wiztui/model.go index 837720c69..38dd1fe06 100644 --- a/internal/cli/wiztui/model.go +++ b/internal/cli/wiztui/model.go @@ -117,6 +117,17 @@ type IndexOutcome struct { // nil/empty in monolith mode, which has no per-repo classify — rows there // fall back entirely to finalizeRows. RepoStats []RepoStat + + // Outstanding names the post-extraction stage(s) (group-algo / links) for + // this group that were still running or pending the instant this outcome + // was produced, phrased in the same vocabulary `grafel status`'s + // annotation/stage_gate lines use — or "" when nothing was outstanding + // (#6047). Only meaningful on a terminal, successful (Err==nil, + // !DaemonDown) outcome; the cli package is responsible for querying the + // daemon's status and leaves it "" when the query itself fails (a + // best-effort caveat, never a reason to fail or delay completion). See + // indexView.outstanding's doc for how it renders. + Outstanding string } // RepoStat is one selected repo's final classified result (see @@ -353,7 +364,13 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, waitEvent(m.evCh) case metricsMsg: - m.idx.rssMB = msg.RSSMB + // rssMB tracks the RUNNING PEAK, never a raw latest sample (#6047) — see + // indexView.rssMB's doc for why an unlabelled instant reading next to a + // frozen "Done" understates the run's actual high-water mark. cpuPct + // stays a plain live reading (no equivalent complaint for CPU%). + if msg.RSSMB > m.idx.rssMB { + m.idx.rssMB = msg.RSSMB + } m.idx.cpuPct = msg.CPUPct if m.idx.done() { // Index screen is finished (Done/Failed) — stop polling rather @@ -412,6 +429,14 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // still on an intermediate phase to Done — its final SSE events may // have arrived after the RPC returned and been dropped (#5340). m.idx.finalizeRows() + // Completion-honesty caveat (#6047): only meaningful when the group + // was genuinely indexed (not DaemonDown — nothing ran, so nothing + // can be outstanding). The cli package already leaves Outstanding "" + // in that case, but gate on it here too so wiztui's own invariant + // doesn't depend on the caller getting that right. + if !o.DaemonDown { + m.idx.outstanding = o.Outstanding + } } m.idx.finishedAt = time.Now() m.scr = scrDone diff --git a/internal/cli/wiztui/model_test.go b/internal/cli/wiztui/model_test.go index fa640fa0c..481cc7fbf 100644 --- a/internal/cli/wiztui/model_test.go +++ b/internal/cli/wiztui/model_test.go @@ -1,6 +1,7 @@ package wiztui import ( + "strings" "testing" "time" @@ -540,6 +541,93 @@ func TestModel_FinalOutcomeAfterInterim_ReachesDoneWithFinalStats(t *testing.T) } } +// TestModel_TerminalOutcome_CarriesOutstandingCaveat is the pinning test for +// #6047 direction 1: a terminal outcome that names an outstanding +// post-extraction stage must have that caveat applied to idx.outstanding — +// and it must render on the done screen (proving the fixture CAN exhibit an +// outstanding stage, not just that the field exists). +func TestModel_TerminalOutcome_CarriesOutstandingCaveat(t *testing.T) { + m := driveToIndexScreen(t, nilIndex) + const caveat = "still running: group-algo, links (communities/pagerank/centrality overlay not yet current; the graph itself is queryable)" + m = m.update(outcomeMsg(IndexOutcome{Entities: 500, Rels: 90, Outstanding: caveat})) + + if m.scr != scrDone { + t.Fatalf("scr = %v, want scrDone", m.scr) + } + if m.idx.outstanding != caveat { + t.Errorf("idx.outstanding = %q, want %q", m.idx.outstanding, caveat) + } + // Use idx.view() (the unframed body), not the outer m.View() — the outer + // frame word-wraps to the terminal width, which would break this long + // caveat across lines and defeat a plain substring check. + if out := m.idx.view(); !strings.Contains(out, caveat) { + t.Errorf("done screen missing outstanding caveat:\n%s", out) + } +} + +// TestModel_TerminalOutcome_NoOutstanding_RendersNoCaveat is the pinning test +// for #6047 direction 2: a terminal outcome with NO outstanding stage +// (Outstanding=="") must leave idx.outstanding empty and the done screen must +// not print any "still running" text — proving the same code path can also +// exhibit the "nothing outstanding" outcome, not just the caveat one. +func TestModel_TerminalOutcome_NoOutstanding_RendersNoCaveat(t *testing.T) { + m := driveToIndexScreen(t, nilIndex) + m = m.update(outcomeMsg(IndexOutcome{Entities: 500, Rels: 90})) + + if m.scr != scrDone { + t.Fatalf("scr = %v, want scrDone", m.scr) + } + if m.idx.outstanding != "" { + t.Errorf("idx.outstanding = %q, want empty", m.idx.outstanding) + } + if out := m.idx.view(); strings.Contains(out, "still running") { + t.Errorf("done screen rendered a caveat with nothing outstanding:\n%s", out) + } +} + +// TestModel_TerminalOutcome_DaemonDown_SuppressesOutstanding: a DaemonDown +// completion (nothing was actually indexed) must never surface an outstanding +// caveat even if the IndexOutcome carried one — there is nothing running to +// name. +func TestModel_TerminalOutcome_DaemonDown_SuppressesOutstanding(t *testing.T) { + m := driveToIndexScreen(t, nilIndex) + m = m.update(outcomeMsg(IndexOutcome{DaemonDown: true, Outstanding: "still running: group-algo (bogus)"})) + + if m.idx.outstanding != "" { + t.Errorf("idx.outstanding = %q, want empty on DaemonDown", m.idx.outstanding) + } +} + +// TestModel_MetricsMsg_TracksPeakNotLatest is the pinning test for #6047's +// memory-readout fix: rssMB must track the RUNNING MAX across polls, not the +// latest sample — a poll reporting a LOWER RSS than a previous one (the +// realistic shape: RSS falls once the enrichment phase finishes and buffers +// are released) must not lower the displayed figure. +func TestModel_MetricsMsg_TracksPeakNotLatest(t *testing.T) { + m := driveToIndexScreen(t, nilIndex) + + m = m.update(metricsMsg(Metrics{RSSMB: 900, CPUPct: 10})) + if m.idx.rssMB != 900 { + t.Fatalf("rssMB = %d, want 900 after first poll", m.idx.rssMB) + } + + m = m.update(metricsMsg(Metrics{RSSMB: 1444, CPUPct: 300})) + if m.idx.rssMB != 1444 { + t.Fatalf("rssMB = %d, want 1444 (the higher mid-run sample)", m.idx.rssMB) + } + + // A later, LOWER sample (RSS falling post-enrichment) must not regress + // the displayed peak. + m = m.update(metricsMsg(Metrics{RSSMB: 900, CPUPct: 5})) + if m.idx.rssMB != 1444 { + t.Errorf("rssMB = %d, want 1444 (must not regress to a later, lower sample)", m.idx.rssMB) + } + // cpuPct stays a live instantaneous reading, unlike rssMB. + if m.idx.cpuPct != 5 { + t.Errorf("cpuPct = %v, want 5 (a live reading, not a peak)", m.idx.cpuPct) + } +} + // TestModel_SilentRepoStillRendersRowAndCompletes is the model-level // regression for the live dropped-row bug: a 3-repo group where the third // repo's progress events never arrive must still render a row for it diff --git a/internal/cli/wiztui/view_test.go b/internal/cli/wiztui/view_test.go index 89428bb37..881e40f77 100644 --- a/internal/cli/wiztui/view_test.go +++ b/internal/cli/wiztui/view_test.go @@ -416,3 +416,20 @@ func TestIndexView_MetricSuffix_PresentAndAbsent(t *testing.T) { t.Errorf("expected \"2.3 GB\" in output:\n%s", bothOut) } } + +// TestIndexView_MetricSuffix_LabelledAsPeak is the pinning test for #6047: the +// RAM readout must be explicitly labelled "peak" — an unlabelled reading next +// to a frozen "Done · 15m55s" reads as the run's total when it is really +// whatever the engine happened to be using at whatever instant was last +// polled, understating the real high-water mark by several times (per the +// issue's measured 0.9GB-at-exit vs 1444MB engine-peak numbers). +func TestIndexView_MetricSuffix_LabelledAsPeak(t *testing.T) { + v := newIndexView("grp", 1) + v.width = 100 + v.foldEvent(progress.Event{RepoSlug: "backend", Phase: progress.PhaseExtractAST, FilesDone: 1, FilesTotal: 10, TS: 1}) + v.rssMB = 2355 + out := v.metricSuffix() + if !strings.Contains(out, "peak 2.3 GB") { + t.Errorf("metricSuffix must label the RSS reading as \"peak\":\n%s", out) + } +} From 1c060a08e96da584fc0a61c0fcf7df51dc36c5bb Mon Sep 17 00:00:00 2001 From: Jorge Cajas Date: Fri, 31 Jul 2026 05:51:42 +0800 Subject: [PATCH 2/3] fix(#6047): finish-early honesty, engine-scoped peak, split-mode signal, analytics stage Address round-2 adversarial review findings on the completion-honesty fix: 1. The "finish early" path (enter pressed from the queryable/interim sub-state) set idx.terminal, which makes view() render doneSummary INSTEAD of the queryableBanner the user was just looking at -- silently dropping the "background enhancement still running" honesty the banner carried. Finishing early is, by construction, always leaving before that pass has acked, so it now unconditionally sets a fixed, honestly-worded caveat (no daemon query available at that point, unlike the normal terminal-outcome path). 2. The "peak" RAM label overclaimed: rssMB is scoped to the engine process alone (process.RSSBytes(os.Getpid()) in statuswriter.go), while extraction runs in separate child processes and is the dominant consumer (measured: extract child 3411 MB vs engine 1444 MB). Relabelled "engine peak" -- true of what it measures, silent about what it doesn't cover. Aggregating the extract children is out of scope here (filed as #6048; their own self-reported RSS is separately broken). 3. attachOutstanding had no test proving the daemon is actually queried and its answer threaded through -- a mutation discarding the fetched status left the whole suite green. Split the bridge into an injectable statusFetcher seam (attachOutstandingWith) and pinned it directly. 4. The split-vs-monolith doc had the branches backwards: GroupAlgoRunning / PendingAlgo / PendingLinks are only ever populated in MONOLITH mode (service.go's s.scheduler != nil branch) -- split mode, the default and the mode the bug was measured in, only ever populates StageGateHolder/Deferred/Barging and the coarse GroupAlgoInFlight count. Corrected the comment and now consume GroupAlgoInFlight as the split-mode-native "an overlay recompute is happening" signal. Also added the "analytics" stage (the post-rebuild quality-metrics scan's foreground barge, `barging=analytics:` in the issue's own repro) which was never checked -- named separately from group-algo/links since it has nothing to do with the community/pagerank/centrality overlay, so it never carries that overlay clause when it's the only outstanding stage. GroupAlgoInFlight does NOT close the pre-existing debounce-armed window (a group-algo pass armed but not yet executing) -- that gap, a stale/absent sidecar, and best-effort ""-on-error being indistinguishable from genuine completion remain accepted, pre-existing plumbing limits, not addressed here. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0111KEDUVWEWm9G5RRnJqXft --- internal/cli/wizard_outstanding.go | 179 ++++++++++++++++++------ internal/cli/wizard_outstanding_test.go | 123 ++++++++++++++++ internal/cli/wiztui/indexview.go | 119 +++++++++++----- internal/cli/wiztui/model.go | 19 ++- internal/cli/wiztui/model_test.go | 50 +++++++ internal/cli/wiztui/view_test.go | 26 ++-- 6 files changed, 420 insertions(+), 96 deletions(-) diff --git a/internal/cli/wizard_outstanding.go b/internal/cli/wizard_outstanding.go index 268c53238..ed530b15f 100644 --- a/internal/cli/wizard_outstanding.go +++ b/internal/cli/wizard_outstanding.go @@ -8,10 +8,12 @@ package cli // awaitSplitCompletion) — i.e. once extraction + the cross-repo link merge // have written graph.fb. The graph genuinely IS queryable at that point (every // structural MCP tool works), but the community/pagerank/centrality overlay -// (group-algo) and, in some orderings, the cross-repo link pass itself are -// stage-gated to run AFTER that ack, in the background, via the SAME -// stage-gate `grafel status` already reports through its annotation/ -// stage_gate lines (see internal/cli/status_annotation.go and +// (group-algo), the cross-repo link pass itself (in some orderings), and the +// post-rebuild quality-metrics scan (cmd/grafel/rebuild_history.go's +// "analytics:" barge, unrelated to the overlay but still real +// outstanding work) are all stage-gated to run AFTER that ack, in the +// background, via the SAME stage-gate `grafel status` already reports through +// its annotation/stage_gate lines (see internal/cli/status_annotation.go and // status_daemon_detail.go). A user or agent that starts querying the instant // the wizard exits gets an overlay that is silently not current, with no // signal from the wizard that anything is outstanding — the same defect class @@ -21,12 +23,53 @@ package cli // turn a ~16-minute run into a ~26-minute one for a large group, for no user // benefit — the graph is already queryable). Instead, query the daemon's // status ONE more time the instant the terminal outcome is available and, if -// group-algo or links is still running/pending for OUR group, attach a caveat -// naming it — in the SAME vocabulary status_annotation.go's +// group-algo, links, or analytics is still running/pending for OUR group, +// attach a caveat naming it — reusing status_annotation.go's // "communities/pagerank/centrality overlay not yet current; the graph itself -// is queryable" line already uses — to wiztui.IndexOutcome.Outstanding. Silent -// (empty string) when nothing is outstanding, which is the common case (a -// small group, or an already-warm overlay). +// is queryable" wording where it actually applies (group-algo/links) — to +// wiztui.IndexOutcome.Outstanding. Silent (empty string) when nothing is +// outstanding, which is the common case (a small group, or an already-warm +// overlay). +// +// #6047 REVIEW ROUND 2 — SPLIT VS. MONOLITH, CORRECTED. An earlier version of +// this file's doc had split/monolith backwards. The actual split: +// - MONOLITH (s.scheduler != nil in Service.Status, service.go:394-453): the +// scheduler lives in the SAME process answering the RPC, so the FULL +// snapshot is available — StageGateHolder/Deferred/Barging AND the coarse +// GroupAlgoRunning/PendingAlgo/PendingLinks lists (which name the exact +// groups) are all populated directly from it. +// - SPLIT (the DEFAULT — service.go:454-495, the `else if` fallback): the +// scheduler lives in the ENGINE process, not the one answering this RPC. +// GroupAlgoRunning/PendingAlgo/PendingLinks are structurally NEVER +// populated here — nothing in the tree ever assigns them in this branch. +// Only StageGateHolder/Deferred/Barging (relayed verbatim from the +// engine's liveness sidecar, StageGateFromStatusFile) and the numeric +// GroupAlgoInFlight (GroupAlgoInFlightFromStatusFile, also sidecar-backed) +// are available. Since split mode is the default AND the mode #6047 was +// measured in, outstandingCaveat must not depend on the coarse lists to +// do anything useful — it keeps checking them (harmless in split mode, +// where they're always empty; load-bearing in monolith mode/tests), but +// ALSO checks GroupAlgoInFlight, the one split-mode-native "is an overlay +// recompute happening somewhere" signal (see its doc: "the pass writes +// the ... overlay AFTER the rebuild has already reported completion, so +// without this the only observable symptom ... is that clusters come back +// empty" — literally #6047's failure mode, from the commit that added the +// signal). +// +// KNOWN IMPRECISION, ACCEPTED: GroupAlgoInFlight is a global COUNT across +// every group the engine happens to be running group-algo for right now — +// it does not name groups, so a wizard finishing group A while an +// unrelated group B's overlay pass happens to be executing will +// (rarely, and only when the operator runs two groups concurrently) +// over-report A as outstanding. Given the alternative in split mode is +// ALWAYS reporting nothing (silently repeating the bug this file fixes), +// this tradeoff is deliberate, not an oversight. +// +// GroupAlgoInFlight also does NOT close the debounce-armed window (a +// group-algo pass armed but not yet actually executing, so not yet +// counted "in flight" by indexstate.Get().GroupAlgoInFlight, which backs +// the sidecar field) — that gap is a separate, pre-existing, accepted +// plumbing limit, not something this file's scope covers. import ( "fmt" @@ -37,71 +80,117 @@ import ( "github.com/cajasmota/grafel/internal/daemon/proto" ) +// graphQueryableCaveat is the exact clause status_annotation.go's +// printAnnotationStatus already prints — reused verbatim (both standalone, +// for a non-overlay stage like analytics, and as the tail of +// outstandingOverlayCaveat below) so the wizard and `grafel status` describe +// the same state in the same words. +const graphQueryableCaveat = "the graph itself is queryable" + // outstandingOverlayCaveat is the exact phrase status_annotation.go's -// printAnnotationStatus already prints for a running/pending group-algo pass — -// reused verbatim here so the wizard and `grafel status` describe the same -// state in the same words. -const outstandingOverlayCaveat = "communities/pagerank/centrality overlay not yet current; the graph itself is queryable" +// printAnnotationStatus already prints for a running/pending group-algo +// pass. Used only when an OVERLAY-affecting stage (group-algo and/or links) +// is actually outstanding — never for analytics alone, which has nothing to +// do with the community/pagerank/centrality overlay (see +// cmd/grafel/rebuild_history.go's appendRebuildHistory: it is a +// quality-metrics history scan, a completely different post-rebuild batch +// that merely happens to share the stage-gate mechanism). +const outstandingOverlayCaveat = "communities/pagerank/centrality overlay not yet current; " + graphQueryableCaveat // outstandingCaveat derives the wizard's completion caveat naming which -// post-extraction stage(s) — group-algo, links, or both — are still running -// or pending for group, purely from a proto.StatusReply snapshot. Pure and -// side-effect-free so it is unit-testable without a daemon. +// post-extraction stage(s) — group-algo, links, analytics, or some +// combination — are still running or pending for group, purely from a +// proto.StatusReply snapshot. Pure and side-effect-free so it is +// unit-testable without a daemon. // -// A stage counts as OUTSTANDING for group if either the coarse +// group-algo/links count as OUTSTANDING for group if either the coarse // running/pending lists name it (GroupAlgoRunning/PendingAlgo/PendingLinks — -// the same fields printAnnotationStatus and printDaemonDetail's scheduler -// line read) or the fine-grained stage gate (StageGateHolder/ -// StageGateDeferred) names "group-algo:" / "links:" — the exact -// holder/deferred naming scheme documented on -// internal/daemon/sched/scheduler.go's StageHolder/StageDeferred fields. -// Checking both catches every mode: split-mode's status snapshot populates -// the coarse lists (Service.Status' snap.PendingAlgo / snap.GroupAlgoRunning -// branch), monolith's populates the stage gate directly (the `g.Holder` / -// `g.Deferred` branch) — see internal/daemon/service.go. +// populated only in MONOLITH mode, see this file's top doc) or the +// fine-grained stage gate (StageGateHolder/StageGateDeferred) names +// "group-algo:" / "links:" (populated in BOTH modes) or — for +// group-algo only, the split-mode-native signal — GroupAlgoInFlight > 0 (see +// this file's top doc for its known imprecision). analytics counts as +// outstanding if StageGateBarging names "analytics:" (the +// quality-metrics history scan's foreground barge registration — see +// cmd/grafel/rebuild_history.go). // -// Returns "" when neither stage is outstanding for group — the caller (see +// Returns "" when nothing is outstanding for group — the caller (see // attachOutstanding) leaves wiztui.IndexOutcome.Outstanding "" in that case, // and indexView renders nothing extra (a caveat, not decoration). func outstandingCaveat(st proto.StatusReply, group string) string { - algoRunning := slices.Contains(st.GroupAlgoRunning, group) || st.StageGateHolder == "group-algo:"+group + algoRunning := slices.Contains(st.GroupAlgoRunning, group) || + st.StageGateHolder == "group-algo:"+group || + st.GroupAlgoInFlight > 0 algoPending := slices.Contains(st.PendingAlgo, group) || slices.Contains(st.StageGateDeferred, "group-algo:"+group) linksRunning := st.StageGateHolder == "links:"+group linksPending := slices.Contains(st.PendingLinks, group) || slices.Contains(st.StageGateDeferred, "links:"+group) + analyticsBarging := slices.Contains(st.StageGateBarging, "analytics:"+group) - var stages []string + var overlayStages []string switch { case algoRunning: - stages = append(stages, "group-algo") + overlayStages = append(overlayStages, "group-algo") case algoPending: - stages = append(stages, "group-algo (pending)") + overlayStages = append(overlayStages, "group-algo (pending)") } switch { case linksRunning: - stages = append(stages, "links") + overlayStages = append(overlayStages, "links") case linksPending: - stages = append(stages, "links (pending)") + overlayStages = append(overlayStages, "links (pending)") } - if len(stages) == 0 { + + var otherStages []string + if analyticsBarging { + otherStages = append(otherStages, "analytics") + } + + allStages := append(append([]string{}, overlayStages...), otherStages...) + if len(allStages) == 0 { return "" } - return fmt.Sprintf("still running: %s (%s)", strings.Join(stages, ", "), outstandingOverlayCaveat) + + // Only claim the overlay is not current when an overlay-affecting stage + // (group-algo/links) is actually in the list — analytics alone must never + // carry that clause, since it has nothing to do with the overlay. + caveat := graphQueryableCaveat + if len(overlayStages) > 0 { + caveat = outstandingOverlayCaveat + } + return fmt.Sprintf("still running: %s (%s)", strings.Join(allStages, ", "), caveat) } -// attachOutstanding queries the daemon's current status and returns the -// completion caveat for group (see outstandingCaveat), or "" on ANY failure — -// a dead/unreachable client, an RPC error, or nothing outstanding. This is a -// best-effort, non-blocking honesty check on an already-successful -// completion, never a reason to fail or delay it: a status query that errors -// degrades to "no caveat shown" exactly like an absent rssMB reading degrades -// to "no readout shown" (see wizard_metrics.go's engineMetricsFuncWith). -func attachOutstanding(c *client.Client, group string) string { - if c == nil { +// statusFetcher mirrors client.Client.Status's signature — the seam +// attachOutstandingWith is tested against without a live daemon (#6047 review +// round 2: the production wiring from attachOutstanding through +// outstandingCaveat had no test proving the daemon is actually queried and +// its answer actually used — replacing attachOutstanding's body with "query, +// discard, return ”" left the whole ./internal/cli suite green). Production +// passes c.Status directly, which already has this exact signature. +type statusFetcher func() (proto.StatusReply, error) + +// attachOutstandingWith calls fetch and folds the result through +// outstandingCaveat for group, or returns "" on ANY failure — a nil fetcher +// or an RPC error. This is the side-effect-free half of attachOutstanding, +// unit-testable with a fake fetcher. +func attachOutstandingWith(fetch statusFetcher, group string) string { + if fetch == nil { return "" } - st, err := c.Status() + st, err := fetch() if err != nil { return "" } return outstandingCaveat(st, group) } + +// attachOutstanding queries the daemon's current status (via c.Status, +// production's statusFetcher) and returns the completion caveat for group — +// best-effort, see attachOutstandingWith's doc; never attempted when c is +// nil (--no-index / daemon-down paths that never dialed). +func attachOutstanding(c *client.Client, group string) string { + if c == nil { + return "" + } + return attachOutstandingWith(c.Status, group) +} diff --git a/internal/cli/wizard_outstanding_test.go b/internal/cli/wizard_outstanding_test.go index 06a2ca74b..19bee0136 100644 --- a/internal/cli/wizard_outstanding_test.go +++ b/internal/cli/wizard_outstanding_test.go @@ -1,6 +1,7 @@ package cli import ( + "errors" "strings" "testing" @@ -20,6 +21,7 @@ func TestOutstandingCaveat_NothingOutstanding_ReturnsEmpty(t *testing.T) { PendingLinks: []string{"other-group"}, StageGateHolder: "group-algo:other-group", StageGateDeferred: []string{"links:other-group"}, + StageGateBarging: []string{"analytics:other-group"}, } got := outstandingCaveat(st, "mygroup") if got != "" { @@ -121,6 +123,78 @@ func TestOutstandingCaveat_BothStagesOutstanding_NamesBoth(t *testing.T) { } } +// TestOutstandingCaveat_GroupAlgoInFlight_ReturnsCaveat pins the SPLIT-MODE +// signal (#6047 review round 2): GroupAlgoInFlight is the ONLY group-algo +// signal ever populated in split mode (the default) — GroupAlgoRunning / +// PendingAlgo / PendingLinks are structurally always empty there (see +// wizard_outstanding.go's top doc). Without consuming this field the wizard +// would silently report nothing outstanding in the exact mode/scenario #6047 +// was measured in, whenever the stage-gate holder/deferred fields also +// happen to be empty (e.g. the debounce-armed window right after the ack). +func TestOutstandingCaveat_GroupAlgoInFlight_ReturnsCaveat(t *testing.T) { + st := proto.StatusReply{GroupAlgoInFlight: 1} + got := outstandingCaveat(st, "mygroup") + if got == "" { + t.Fatal("outstandingCaveat = \"\", want a non-empty caveat") + } + if !strings.Contains(got, "group-algo") { + t.Errorf("caveat missing stage name %q: %q", "group-algo", got) + } + if !strings.Contains(got, outstandingOverlayCaveat) { + t.Errorf("caveat must reuse `grafel status`'s exact overlay wording:\ngot: %q\nwant substring: %q", got, outstandingOverlayCaveat) + } +} + +// TestOutstandingCaveat_GroupAlgoInFlightZero_NoCaveat pins the other +// direction for the same field: GroupAlgoInFlight==0 (the JSON zero value, +// indistinguishable from "field never populated") must NOT, by itself, +// produce a caveat. +func TestOutstandingCaveat_GroupAlgoInFlightZero_NoCaveat(t *testing.T) { + got := outstandingCaveat(proto.StatusReply{GroupAlgoInFlight: 0}, "mygroup") + if got != "" { + t.Errorf("outstandingCaveat = %q, want empty when GroupAlgoInFlight==0", got) + } +} + +// TestOutstandingCaveat_AnalyticsBarging_ReturnsCaveat_NoOverlayClause pins +// the analytics stage (#6047 review round 2 — the issue's own reproduction +// showed `barging=analytics:` and it was never checked). analytics is +// the post-rebuild quality-metrics history scan (cmd/grafel/rebuild_history.go), +// NOT part of the community/pagerank/centrality overlay — so when it is the +// ONLY outstanding stage, the caveat must name it WITHOUT claiming the +// overlay is not current (that claim would itself be dishonest). +func TestOutstandingCaveat_AnalyticsBarging_ReturnsCaveat_NoOverlayClause(t *testing.T) { + st := proto.StatusReply{StageGateBarging: []string{"analytics:mygroup"}} + got := outstandingCaveat(st, "mygroup") + if !strings.Contains(got, "analytics") { + t.Errorf("caveat = %q, want it to name analytics", got) + } + if strings.Contains(got, "communities/pagerank/centrality") { + t.Errorf("caveat = %q, must NOT claim the overlay is stale when analytics is the only outstanding stage", got) + } + if !strings.Contains(got, graphQueryableCaveat) { + t.Errorf("caveat = %q, want it to still reuse the graph-queryable clause", got) + } +} + +// TestOutstandingCaveat_AnalyticsPlusGroupAlgo_NamesBothWithOverlayClause +// pins the combined case: when an overlay-affecting stage IS also +// outstanding alongside analytics, the overlay clause belongs in the caveat +// and analytics is named alongside it. +func TestOutstandingCaveat_AnalyticsPlusGroupAlgo_NamesBothWithOverlayClause(t *testing.T) { + st := proto.StatusReply{ + StageGateBarging: []string{"analytics:mygroup"}, + GroupAlgoRunning: []string{"mygroup"}, + } + got := outstandingCaveat(st, "mygroup") + if !strings.Contains(got, "analytics") || !strings.Contains(got, "group-algo") { + t.Errorf("caveat = %q, want both analytics and group-algo named", got) + } + if !strings.Contains(got, outstandingOverlayCaveat) { + t.Errorf("caveat = %q, want the overlay clause present (group-algo is outstanding)", got) + } +} + // TestAttachOutstanding_NilClient_ReturnsEmpty: attachOutstanding must never // panic or block on a nil client (e.g. --no-index / daemon-down paths that // never dial) — it degrades to "no caveat", same as an absent RSS reading. @@ -129,3 +203,52 @@ func TestAttachOutstanding_NilClient_ReturnsEmpty(t *testing.T) { t.Errorf("attachOutstanding(nil, ...) = %q, want empty", got) } } + +// TestAttachOutstandingWith_QueriesAndAppliesCaveat is the pinning test for +// the attachOutstanding→outstandingCaveat BRIDGE (#6047 review round 2 +// finding 3): a mutation that replaces attachOutstanding's body with "call +// the fetcher, discard the result, return ”" left the entire ./internal/cli +// suite green, because nothing exercised the bridge — only the nil-client +// short-circuit and the pure outstandingCaveat function were pinned +// separately. This drives attachOutstandingWith with a fake fetcher standing +// in for c.Status, proving the fetched StatusReply is actually threaded +// through to the returned caveat. +func TestAttachOutstandingWith_QueriesAndAppliesCaveat(t *testing.T) { + fetch := func() (proto.StatusReply, error) { + return proto.StatusReply{GroupAlgoRunning: []string{"mygroup"}}, nil + } + got := attachOutstandingWith(fetch, "mygroup") + if got == "" { + t.Fatal("attachOutstandingWith = \"\", want the fetched status's caveat") + } + if !strings.Contains(got, "group-algo") { + t.Errorf("attachOutstandingWith = %q, want it to name group-algo", got) + } +} + +// TestAttachOutstandingWith_NothingOutstanding_ReturnsEmpty pins the other +// direction of the bridge: a fetched status with nothing outstanding must +// still flow through to an empty caveat (not a stale/fabricated one). +func TestAttachOutstandingWith_NothingOutstanding_ReturnsEmpty(t *testing.T) { + fetch := func() (proto.StatusReply, error) { return proto.StatusReply{}, nil } + if got := attachOutstandingWith(fetch, "mygroup"); got != "" { + t.Errorf("attachOutstandingWith = %q, want empty", got) + } +} + +// TestAttachOutstandingWith_FetchError_ReturnsEmpty: an RPC error from the +// fetcher must degrade to "no caveat", never panic or propagate. +func TestAttachOutstandingWith_FetchError_ReturnsEmpty(t *testing.T) { + fetch := func() (proto.StatusReply, error) { return proto.StatusReply{}, errors.New("dial: connection refused") } + if got := attachOutstandingWith(fetch, "mygroup"); got != "" { + t.Errorf("attachOutstandingWith = %q, want empty on fetch error", got) + } +} + +// TestAttachOutstandingWith_NilFetcher_ReturnsEmpty: a nil fetcher (mirrors +// attachOutstanding's nil-client short-circuit) must never panic. +func TestAttachOutstandingWith_NilFetcher_ReturnsEmpty(t *testing.T) { + if got := attachOutstandingWith(nil, "mygroup"); got != "" { + t.Errorf("attachOutstandingWith(nil, ...) = %q, want empty", got) + } +} diff --git a/internal/cli/wiztui/indexview.go b/internal/cli/wiztui/indexview.go index 197e770ba..0256a3f8c 100644 --- a/internal/cli/wiztui/indexview.go +++ b/internal/cli/wiztui/indexview.go @@ -85,47 +85,83 @@ type indexView struct { // bgAnimDir is the current direction (+1 or -1) of the bgPct sweep. bgAnimDir float64 - // rssMB / cpuPct are the engine process's CPU/RAM readout (wizard CPU/RAM + // rssMB / cpuPct are the ENGINE process's CPU/RAM readout (wizard CPU/RAM // readout — see internal/statusfile.File's RSSMB/CPUPct doc), polled // periodically from the engine-liveness status-plane sidecar via Model's // metricsFn (see model.go). Zero/absent means "unknown or not yet polled" // and the readout is omitted entirely — never rendered as a misleading // 0%/0.0 GB. // - // rssMB is the RUNNING PEAK, not the latest instantaneous sample (#6047): - // Model's metricsMsg handler only ever raises it, never lowers it. A raw - // live reading taken at the moment the wizard exits is whatever the engine - // happened to be using at that instant — typically well past its own - // multi-GB enrichment-phase high-water mark, since RSS falls once - // enrichment finishes and buffers are released — and next to "Done · - // 15m55s" it reads as "this run used 0.9 GB" when the run's actual peak - // was several times that. Tracking the max of every poll and labelling it - // "peak" (see metricSuffix) reports the same honest in-process metric - // (memtrace's rss_bytes, which tracks `ps` within a few MB — this is a - // peak-vs-instant LABELLING fix, not a measurement fix) as what the run - // actually used, not a random late sample of it. cpuPct stays a live - // instantaneous reading (best-effort, independently omittable) — there is - // no equivalent "peak CPU%" complaint from #6047, and peak-CPU% is a much + // rssMB is the RUNNING PEAK of that ONE process, not the latest + // instantaneous sample (#6047 round 1) and NOT the whole run's footprint + // (#6047 round 2 correction): Model's metricsMsg handler only ever raises + // it, never lowers it, so it tracks the engine's own high-water mark + // instead of whatever it happened to be using at the exact instant the + // wizard polled last. + // + // It is still explicitly SCOPED to the engine (internal/daemon/ + // statuswriter.go populates the sidecar field from + // process.RSSBytes(os.Getpid()) — the writing process alone), which is + // NOT the whole run: extraction runs in separate child OS processes and + // is the dominant consumer (measured on #6047's own repro: extract child + // peak 3411 MB vs. engine peak 1444 MB, whole-installation peak 3525 MB — + // the engine alone is under half the true peak). Aggregating the extract + // children's own RSS is NOT done here: their self-reported figure + // (extract/subproc.go, runtime.MemStats.Sys) excludes the CGO/tree-sitter + // heap that dominates that process, so it would need its own fix first — + // filed separately as #6048, out of scope here. metricSuffix therefore + // labels this reading "engine peak", not just "peak" — an unqualified + // "peak" would be a second, differently-wrong claim about the run's + // actual maximum (understating it by roughly the same ~2.5x the original + // instantaneous-at-exit reading did). cpuPct stays a live instantaneous + // engine reading (best-effort, independently omittable) — there is no + // equivalent "peak CPU%" complaint from #6047, and peak-CPU% is a much // noisier, less meaningful number than peak RSS. rssMB int64 cpuPct float64 - // outstanding names the post-extraction stage(s) (group-algo / links) for - // this group that were still running or pending the instant the wizard's - // terminal outcome landed, in the same vocabulary `grafel status`'s - // annotation/stage_gate lines use (#6047) — or "" when nothing was - // outstanding (the common case: a small group, or an already-warm - // overlay). The wizard deliberately does NOT block on this: the graph - // itself is queryable the instant extraction + the cross-repo link merge - // finish, and waiting for the community/pagerank/centrality overlay too - // would turn a ~16-minute run into a ~26-minute one for no user benefit. - // This is purely a completion-honesty caveat appended to doneSummary, set - // once from IndexOutcome.Outstanding on the terminal outcome — never - // updated afterward (the wizard has already exited its polling loops by - // then). + // outstanding names the post-extraction stage(s) (group-algo / links / + // analytics) for this group that were still running or pending the + // instant the wizard's terminal outcome landed, in the same vocabulary + // `grafel status`'s annotation/stage_gate lines use (#6047) — or "" when + // nothing was outstanding (the common case: a small group, or an + // already-warm overlay). The wizard deliberately does NOT block on this: + // the graph itself is queryable the instant extraction + the cross-repo + // link merge finish, and waiting for the community/pagerank/centrality + // overlay too would turn a ~16-minute run into a ~26-minute one for no + // user benefit. This is purely a completion-honesty caveat appended to + // doneSummary, set once — either from IndexOutcome.Outstanding on a + // normal terminal outcome (a fresh daemon status query, see + // wizard_outstanding.go), or to earlyFinishOutstandingCaveat when the + // user finishes EARLY via enter from the queryable sub-state (see + // Model's updateKey scrIndex case — that path has no outcome to read + // Outstanding from, but by construction — queryable && !terminal — the + // background enhancement pass has NOT yet acked, so the caveat is + // unconditional there, not queried) — never updated afterward (the + // wizard has already exited its polling loops by then). outstanding string } +// earlyFinishOutstandingCaveat is the completion-honesty caveat (#6047 round +// 2) shown when the user finishes EARLY from the queryable/interim sub-state +// (enter pressed before the background enhancement pass has acked — see +// Model's updateKey scrIndex case). Before enter, that sub-state is rendered +// by queryableBanner ("Graph queryable ... enhancing relationships in the +// background"); pressing enter sets v.terminal, which makes view() dispatch +// to doneSummary INSTEAD (see view()'s v.terminal/v.queryable branch order) +// — so without this, the banner's honesty is simply discarded the moment the +// user acts on it, landing on a screen that claims plain "Done" while the +// same background work the banner just described is still running. +// +// Unlike the normal terminal-outcome caveat (computed by the cli package +// from a fresh daemon status query and able to name specific stages — see +// wizard_outstanding.go's outstandingCaveat), finishing early has, BY +// CONSTRUCTION, no such query available: queryable && !terminal means the +// engine has not yet acked the background enhancement pass, full stop, so +// this is a fixed, honest caveat rather than a queried one — phrased with +// the SAME "graph itself is queryable" wording `grafel status` uses. +const earlyFinishOutstandingCaveat = "background enhancement still running (communities/pagerank/centrality overlay not yet current; the graph itself is queryable)" + func newIndexView(group string, expectedRepos int) indexView { b := progress.New( // Blue → teal/green gradient (matches the light-blue accent), replacing @@ -387,14 +423,21 @@ func (v indexView) renderRow(r Row, spinnerFrame string) string { // bar sits near 100% for a long stretch) is still doing real work, not stuck // (motivation for this whole feature). // -// The RAM figure is explicitly labelled "peak" (#6047): v.rssMB is the max of -// every poll this run has seen (see indexView.rssMB's doc), not a live instant -// reading, and next to a frozen "Done · 15m55s" an unlabelled instant reads as -// the run's total when it is really whatever the engine happened to be using -// at the moment the wizard exited — often a fraction of the real high-water -// mark. cpuPct stays a live instantaneous reading, so it keeps its plain "CPU" -// label rather than "peak CPU" (no equivalent complaint, and a live number -// still reads correctly as a live number even while indexing is ongoing). +// The RAM figure is explicitly labelled "engine peak" (#6047, corrected in +// round 2 review from a bare "peak"): v.rssMB is the max of every poll this +// run has seen (see indexView.rssMB's doc) — fixing the original "live +// instant read as a total" defect — but it is ONLY the wizard-owning engine +// process, not the whole run. Extraction runs in separate child OS processes +// and is the DOMINANT consumer (measured on #6047's own repro: extract child +// peak 3411 MB vs. engine peak 1444 MB): an unqualified "peak 1.4 GB" would +// be a new, still-wrong, now-FALSIFIABLE claim about the run's actual +// maximum — worse than the original unlabelled instant, not better. "engine +// peak" is the honest, cheap label: true of what it names, silent about what +// it doesn't cover (aggregating the extract children is #6048, not this +// file's scope — see indexView.rssMB's doc for why). cpuPct stays a live +// instantaneous reading, so it keeps its plain "CPU" label rather than "peak +// CPU" (no equivalent complaint, and a live number still reads correctly as +// a live number even while indexing is ongoing). // // Omits gracefully: an absent/zero rssMB (old status file predating this // field, engine metric read failed, or no poll has landed yet) returns "" and @@ -409,9 +452,9 @@ func (v indexView) metricSuffix() string { gb := float64(v.rssMB) / 1024.0 var text string if v.cpuPct > 0 { - text = fmt.Sprintf("%s CPU %.0f%% %s peak %.1f GB", g.MidDot, v.cpuPct, g.MidDot, gb) + text = fmt.Sprintf("%s CPU %.0f%% %s engine peak %.1f GB", g.MidDot, v.cpuPct, g.MidDot, gb) } else { - text = fmt.Sprintf("%s peak %.1f GB", g.MidDot, gb) + text = fmt.Sprintf("%s engine peak %.1f GB", g.MidDot, gb) } return " " + rowCountStyle.Render(text) } diff --git a/internal/cli/wiztui/model.go b/internal/cli/wiztui/model.go index 38dd1fe06..5efc4f5b7 100644 --- a/internal/cli/wiztui/model.go +++ b/internal/cli/wiztui/model.go @@ -364,10 +364,13 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, waitEvent(m.evCh) case metricsMsg: - // rssMB tracks the RUNNING PEAK, never a raw latest sample (#6047) — see - // indexView.rssMB's doc for why an unlabelled instant reading next to a - // frozen "Done" understates the run's actual high-water mark. cpuPct - // stays a plain live reading (no equivalent complaint for CPU%). + // rssMB tracks the RUNNING PEAK of the engine process, never a raw + // latest sample (#6047) — see indexView.rssMB's doc for why an + // unlabelled instant reading next to a frozen "Done" understates even + // the ENGINE's own high-water mark, let alone the whole run's (rssMB + // is scoped to the engine only — see the same doc for why "engine + // peak" is the honest label, not just "peak"). cpuPct stays a plain + // live reading (no equivalent complaint for CPU%). if msg.RSSMB > m.idx.rssMB { m.idx.rssMB = msg.RSSMB } @@ -502,6 +505,14 @@ func (m Model) updateKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { // path so finishing early is consistent with waiting. m.idx.applyRepoStats(m.idx.interimRepoStats) m.idx.finalizeRows() + // Completion-honesty caveat (#6047 round 2): finishing early is, + // BY DEFINITION, leaving before the background enhancement pass + // has acked (queryable && !terminal means exactly that) — the + // queryableBanner the user was just looking at said so, and + // setting v.terminal above makes view() replace that banner with + // doneSummary. Without this line the caveat that banner carried + // is silently dropped the instant the user acts on it. + m.idx.outstanding = earlyFinishOutstandingCaveat m.idx.finishedAt = time.Now() m.scr = scrDone m.step = StepDone diff --git a/internal/cli/wiztui/model_test.go b/internal/cli/wiztui/model_test.go index 481cc7fbf..b03d24c8f 100644 --- a/internal/cli/wiztui/model_test.go +++ b/internal/cli/wiztui/model_test.go @@ -463,6 +463,56 @@ func TestModel_EnterInQueryableState_FinishesWithInterimStats(t *testing.T) { } } +// TestModel_EnterEarly_CarriesOutstandingCaveat is the pinning test for #6047 +// round 2 finding 1 (reviewer probe, reproduced verbatim): pressing enter +// from the queryable/interim sub-state (background enhancement pass has, BY +// CONSTRUCTION, not yet acked) must land on a done screen that still says so +// — not a bare "Done" that is strictly less honest than the queryableBanner +// the user was just looking at. +func TestModel_EnterEarly_CarriesOutstandingCaveat(t *testing.T) { + m := driveToIndexScreen(t, nilIndex) + m = m.update(outcomeMsg(IndexOutcome{Interim: true, Entities: 777, Rels: 33})) + if m.scr != scrIndex { + t.Fatalf("scr = %v, want scrIndex after interim", m.scr) + } + + // BEFORE enter: the banner is on screen and honestly says enhancement is + // still running. + before := m.idx.view() + if !strings.Contains(before, "enhancing relationships in the background") { + t.Fatalf("queryableBanner missing before enter:\n%s", before) + } + + m = m.update(key("enter")) + + if m.scr != scrDone { + t.Fatalf("scr = %v, want scrDone after enter in queryable state", m.scr) + } + if m.idx.outstanding == "" { + t.Fatal("idx.outstanding empty after finishing early — the banner's honesty was silently dropped") + } + after := m.idx.view() + if !strings.Contains(after, "still running") { + t.Errorf("done screen after early finish must still name outstanding work:\n%s", after) + } +} + +// TestModel_NormalTerminalOutcome_DoesNotUseEarlyFinishCaveat proves the +// early-finish caveat is scoped to the enter-early path only: a NORMAL +// terminal outcome (no interim, straight to Done, nothing outstanding) must +// render no caveat at all — the fixture that pins the opposite direction of +// TestModel_EnterEarly_CarriesOutstandingCaveat. +func TestModel_NormalTerminalOutcome_DoesNotUseEarlyFinishCaveat(t *testing.T) { + m := driveToIndexScreen(t, nilIndex) + m = m.update(outcomeMsg(IndexOutcome{Entities: 777, Rels: 33})) + if m.scr != scrDone { + t.Fatalf("scr = %v, want scrDone", m.scr) + } + if m.idx.outstanding != "" { + t.Errorf("idx.outstanding = %q, want empty on a plain terminal outcome with nothing outstanding", m.idx.outstanding) + } +} + // TestModel_EnterEarlyAppliesInterimRepoStats: when the user finishes early // from the queryable state, the interim outcome's per-repo classify stats must // be overlaid onto the rows — otherwise a repo that emitted zero progress diff --git a/internal/cli/wiztui/view_test.go b/internal/cli/wiztui/view_test.go index 881e40f77..84fa1f874 100644 --- a/internal/cli/wiztui/view_test.go +++ b/internal/cli/wiztui/view_test.go @@ -417,19 +417,27 @@ func TestIndexView_MetricSuffix_PresentAndAbsent(t *testing.T) { } } -// TestIndexView_MetricSuffix_LabelledAsPeak is the pinning test for #6047: the -// RAM readout must be explicitly labelled "peak" — an unlabelled reading next -// to a frozen "Done · 15m55s" reads as the run's total when it is really -// whatever the engine happened to be using at whatever instant was last -// polled, understating the real high-water mark by several times (per the -// issue's measured 0.9GB-at-exit vs 1444MB engine-peak numbers). -func TestIndexView_MetricSuffix_LabelledAsPeak(t *testing.T) { +// TestIndexView_MetricSuffix_LabelledAsEnginePeak is the pinning test for +// #6047 (corrected in round 2 review): the RAM readout must be explicitly +// labelled "engine peak", not a bare "peak" — v.rssMB is scoped to the +// engine process alone (internal/daemon/statuswriter.go populates it from +// process.RSSBytes(os.Getpid())), while extraction runs in separate child OS +// processes and is the dominant consumer (issue's own repro: extract child +// peak 3411 MB vs. engine peak 1444 MB). An unqualified "peak 1.4 GB" would +// be a second, still-wrong, now-falsifiable claim about the run's actual +// maximum — the label must say exactly what it measures. +func TestIndexView_MetricSuffix_LabelledAsEnginePeak(t *testing.T) { v := newIndexView("grp", 1) v.width = 100 v.foldEvent(progress.Event{RepoSlug: "backend", Phase: progress.PhaseExtractAST, FilesDone: 1, FilesTotal: 10, TS: 1}) v.rssMB = 2355 out := v.metricSuffix() - if !strings.Contains(out, "peak 2.3 GB") { - t.Errorf("metricSuffix must label the RSS reading as \"peak\":\n%s", out) + if !strings.Contains(out, "engine peak 2.3 GB") { + t.Errorf("metricSuffix must label the RSS reading as \"engine peak\":\n%s", out) + } + // A bare, unscoped "peak" (without "engine") would silently reintroduce + // the over-broad claim this fix corrects. + if strings.Count(out, "peak") != 1 { + t.Errorf("expected exactly one \"peak\" occurrence (as part of \"engine peak\"):\n%s", out) } } From 5f5de234b4c6ca0346849e667ba1ea65d6e2732f Mon Sep 17 00:00:00 2001 From: Jorge Cajas Date: Fri, 31 Jul 2026 06:29:21 +0800 Subject: [PATCH 3/3] fix(#6047): guard GroupAlgoInFlight against a non-empty running list, close the last seam Address round-3 adversarial review findings: REQUIRED. GroupAlgoInFlight is a process-wide count, not group-scoped (indexstate.go backs it with a bare atomic.Int64), while GroupAlgoRunning, when populated (monolith mode), is exact. The round-2 `|| GroupAlgoInFlight > 0` let the imprecise count override the precise list in the one mode where the precise list exists: group A finishes and drains, an unrelated group B's pass is still executing, and the wizard reported A as outstanding -- exactly the direction the issue forbids. Now falls back to the count only when the list is empty (split mode, or a genuinely idle monolith), never when it's non-empty and simply omits the group. Also corrected the doc upward: GroupAlgoInFlight's two increment sites cover both "executing" and "gate-deferred" (scheduler.go:2391 and :2222), so the guarded fallback also catches the deferred state, not just execution. RECOMMENDED, done. toIndexOutcome took a *client.Client, so nothing but its trivial nil-check was ever exercised by a test -- a mutation bypassing the whole caveat computation and hardcoding "" left the suite green. It now takes a statusFetcher (c.Status already has that signature) at all three production call sites, and the two wizard_split_progress_test.go sites pass a fake instead of nil. Removed the now-dead attachOutstanding(*client.Client) wrapper along with it. Re-ran the exact bypass mutation the prior round's review found surviving; it now dies via the new TestToIndexOutcome_ThreadsCaveatFromFetcher. Also added a one-line source comment on the enter-early completion path noting the millisecond-scale window where a real terminal outcome can already be sitting unread on outCh when the keypress lands (LOW, errs toward caution, left as acknowledged rather than chased). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0111KEDUVWEWm9G5RRnJqXft --- internal/cli/wizard_outstanding.go | 106 +++++++++++-------- internal/cli/wizard_outstanding_test.go | 113 +++++++++++++++++---- internal/cli/wizard_split_progress_test.go | 15 ++- internal/cli/wizard_tui_run.go | 25 +++-- internal/cli/wiztui/model.go | 11 ++ 5 files changed, 200 insertions(+), 70 deletions(-) diff --git a/internal/cli/wizard_outstanding.go b/internal/cli/wizard_outstanding.go index ed530b15f..9739a3830 100644 --- a/internal/cli/wizard_outstanding.go +++ b/internal/cli/wizard_outstanding.go @@ -56,27 +56,51 @@ package cli // empty" — literally #6047's failure mode, from the commit that added the // signal). // -// KNOWN IMPRECISION, ACCEPTED: GroupAlgoInFlight is a global COUNT across -// every group the engine happens to be running group-algo for right now — -// it does not name groups, so a wizard finishing group A while an -// unrelated group B's overlay pass happens to be executing will -// (rarely, and only when the operator runs two groups concurrently) -// over-report A as outstanding. Given the alternative in split mode is -// ALWAYS reporting nothing (silently repeating the bug this file fixes), -// this tradeoff is deliberate, not an oversight. +// GUARDED, NOT UNCONDITIONAL (#6047 review round 3 — an unguarded `||` +// shipped in round 2 was wrong): GroupAlgoInFlight is a global COUNT +// across every group the engine happens to be running group-algo for +// right now (indexstate.go's backing field is a bare atomic.Int64, not +// keyed by group) — it does not name groups. In MONOLITH mode +// GroupAlgoRunning is populated too, and is exact +// (service.go:453 sets GroupAlgoInFlight = len(snap.GroupAlgoRunning) — +// the two fields describe the SAME snapshot, one as names, one as a +// count). An unguarded `|| GroupAlgoInFlight > 0` therefore let the +// imprecise count override the precise list in the one mode where the +// precise list exists and is correct: group A finishes (drained, +// genuinely absent from GroupAlgoRunning) while unrelated group B's pass +// is executing, and the wizard reported A as outstanding — exactly the +// direction the issue forbids ("must not report an outstanding stage +// that has already drained"). outstandingCaveat therefore only falls +// back to the count when the list is EMPTY +// (len(st.GroupAlgoRunning) == 0 && st.GroupAlgoInFlight > 0): empty +// means split mode (structurally always empty there) or a genuinely idle +// monolith (nothing running, so a stray positive count would itself be a +// bug elsewhere) — never "monolith with an unrelated group's pass mid- +// flight", which is exactly the case a non-empty list correctly rules +// group out of. // -// GroupAlgoInFlight also does NOT close the debounce-armed window (a -// group-algo pass armed but not yet actually executing, so not yet -// counted "in flight" by indexstate.Get().GroupAlgoInFlight, which backs -// the sidecar field) — that gap is a separate, pre-existing, accepted -// plumbing limit, not something this file's scope covers. +// GroupAlgoInFlight COVERS MORE THAN THE COMMENT ORIGINALLY CLAIMED: its +// two increment sites are scheduler.go:2391 (the pass begins actually +// executing) and scheduler.go:2222 (markGroupAlgoDeferredLocked — the +// pass was ARMED but got turned away at the gate, i.e. gate-deferred). +// So in split mode the guarded fallback also catches the gate-deferred +// state, not just "actively executing" — which is precisely the state +// the issue's own repro captured (`stage_gate: ... deferred=group-algo: +// ,links:`). It still does NOT fire at timer-arm time, +// before the pass has reached the gate at all — that debounce-armed +// window remains open (see below). +// +// GroupAlgoInFlight does NOT close the debounce-armed window (a +// group-algo pass armed but not yet having reached the gate at all, so +// neither executing nor deferred, so not yet counted "in flight") — that +// narrower gap is a separate, pre-existing, accepted plumbing limit, not +// something this file's scope covers. import ( "fmt" "slices" "strings" - "github.com/cajasmota/grafel/internal/daemon/client" "github.com/cajasmota/grafel/internal/daemon/proto" ) @@ -108,19 +132,23 @@ const outstandingOverlayCaveat = "communities/pagerank/centrality overlay not ye // populated only in MONOLITH mode, see this file's top doc) or the // fine-grained stage gate (StageGateHolder/StageGateDeferred) names // "group-algo:" / "links:" (populated in BOTH modes) or — for -// group-algo only, the split-mode-native signal — GroupAlgoInFlight > 0 (see -// this file's top doc for its known imprecision). analytics counts as -// outstanding if StageGateBarging names "analytics:" (the -// quality-metrics history scan's foreground barge registration — see -// cmd/grafel/rebuild_history.go). +// group-algo only, and ONLY when GroupAlgoRunning is empty (see this file's +// top doc for why an unguarded check is wrong) — the split-mode-native +// fallback signal GroupAlgoInFlight > 0. analytics counts as outstanding if +// StageGateBarging names "analytics:" (the quality-metrics history +// scan's foreground barge registration — see cmd/grafel/rebuild_history.go). // // Returns "" when nothing is outstanding for group — the caller (see -// attachOutstanding) leaves wiztui.IndexOutcome.Outstanding "" in that case, -// and indexView renders nothing extra (a caveat, not decoration). +// attachOutstandingWith) leaves wiztui.IndexOutcome.Outstanding "" in that +// case, and indexView renders nothing extra (a caveat, not decoration). func outstandingCaveat(st proto.StatusReply, group string) string { algoRunning := slices.Contains(st.GroupAlgoRunning, group) || st.StageGateHolder == "group-algo:"+group || - st.GroupAlgoInFlight > 0 + // Fall back to the coarse (unnamed, global) count ONLY when the + // precise list is empty — a non-empty GroupAlgoRunning that omits + // group means group's pass has genuinely drained, and the count must + // not override that (#6047 review round 3). + (len(st.GroupAlgoRunning) == 0 && st.GroupAlgoInFlight > 0) algoPending := slices.Contains(st.PendingAlgo, group) || slices.Contains(st.StageGateDeferred, "group-algo:"+group) linksRunning := st.StageGateHolder == "links:"+group linksPending := slices.Contains(st.PendingLinks, group) || slices.Contains(st.StageGateDeferred, "links:"+group) @@ -161,18 +189,25 @@ func outstandingCaveat(st proto.StatusReply, group string) string { } // statusFetcher mirrors client.Client.Status's signature — the seam -// attachOutstandingWith is tested against without a live daemon (#6047 review -// round 2: the production wiring from attachOutstanding through -// outstandingCaveat had no test proving the daemon is actually queried and -// its answer actually used — replacing attachOutstanding's body with "query, -// discard, return ”" left the whole ./internal/cli suite green). Production -// passes c.Status directly, which already has this exact signature. +// attachOutstandingWith is tested against without a live daemon (#6047 +// review round 2: an earlier attachOutstanding(c *client.Client, group +// string) took the concrete client type, so nothing but its own trivial +// nil-check was exercised by any test — a mutation that bypassed the whole +// computation and returned "" still left the suite green. #6047 review round +// 3 closed that seam: toIndexOutcome (wizard_tui_run.go) now takes a +// statusFetcher directly instead of a *client.Client, and passes c.Status — +// which already has this exact signature — at all three of its call sites. +// wizard_split_progress_test.go's toIndexOutcome tests pass a fake +// statusFetcher for the same reason, so production and tests now go through +// the identical seam). type statusFetcher func() (proto.StatusReply, error) // attachOutstandingWith calls fetch and folds the result through // outstandingCaveat for group, or returns "" on ANY failure — a nil fetcher -// or an RPC error. This is the side-effect-free half of attachOutstanding, -// unit-testable with a fake fetcher. +// (--no-index / daemon-down paths that never dialed a client to take +// c.Status from) or an RPC error. This is the wizard's ONLY entry point into +// this file's completion-honesty caveat — best-effort, unit-testable with a +// fake fetcher. func attachOutstandingWith(fetch statusFetcher, group string) string { if fetch == nil { return "" @@ -183,14 +218,3 @@ func attachOutstandingWith(fetch statusFetcher, group string) string { } return outstandingCaveat(st, group) } - -// attachOutstanding queries the daemon's current status (via c.Status, -// production's statusFetcher) and returns the completion caveat for group — -// best-effort, see attachOutstandingWith's doc; never attempted when c is -// nil (--no-index / daemon-down paths that never dialed). -func attachOutstanding(c *client.Client, group string) string { - if c == nil { - return "" - } - return attachOutstandingWith(c.Status, group) -} diff --git a/internal/cli/wizard_outstanding_test.go b/internal/cli/wizard_outstanding_test.go index 19bee0136..d66554a1a 100644 --- a/internal/cli/wizard_outstanding_test.go +++ b/internal/cli/wizard_outstanding_test.go @@ -5,6 +5,7 @@ import ( "strings" "testing" + "github.com/cajasmota/grafel/internal/cli/wiztui" "github.com/cajasmota/grafel/internal/daemon/proto" ) @@ -22,6 +23,11 @@ func TestOutstandingCaveat_NothingOutstanding_ReturnsEmpty(t *testing.T) { StageGateHolder: "group-algo:other-group", StageGateDeferred: []string{"links:other-group"}, StageGateBarging: []string{"analytics:other-group"}, + // A non-empty GroupAlgoRunning naming only other-group must NOT let + // the coarse GroupAlgoInFlight count override it for mygroup (#6047 + // review round 3 — see TestOutstandingCaveat_GroupAlgoInFlight_ + // DoesNotOverrideNonEmptyRunningList for the dedicated regression). + GroupAlgoInFlight: 1, } got := outstandingCaveat(st, "mygroup") if got != "" { @@ -145,6 +151,43 @@ func TestOutstandingCaveat_GroupAlgoInFlight_ReturnsCaveat(t *testing.T) { } } +// TestOutstandingCaveat_GroupAlgoInFlight_DoesNotOverrideNonEmptyRunningList +// is the REQUIRED regression for #6047 review round 3: GroupAlgoInFlight is +// a process-wide count (indexstate.go's backing field is a bare +// atomic.Int64, not keyed by group), while GroupAlgoRunning — when +// populated, i.e. MONOLITH mode — is exact and names groups. Reviewer's +// probe, reproduced verbatim: an operator indexes group B via the daemon; +// the wizard finishes group A, which has fully drained (genuinely absent +// from GroupAlgoRunning); the count is still positive because of B. The +// wizard must NOT report group-algo outstanding for A — the issue text +// explicitly forbids reporting a stage "that has already drained". +func TestOutstandingCaveat_GroupAlgoInFlight_DoesNotOverrideNonEmptyRunningList(t *testing.T) { + st := proto.StatusReply{ + GroupAlgoRunning: []string{"someone-elses-group"}, + GroupAlgoInFlight: 1, + } + got := outstandingCaveat(st, "mygroup") + if got != "" { + t.Errorf("outstandingCaveat = %q, want empty — mygroup is absent from a NON-EMPTY GroupAlgoRunning, so the coarse count must not override it", got) + } +} + +// TestOutstandingCaveat_GroupAlgoInFlight_FallsBackWhenRunningListEmpty pins +// the other direction of the same guard: when GroupAlgoRunning is EMPTY +// (split mode, structurally always empty — or a genuinely idle monolith), +// GroupAlgoInFlight > 0 must still fall back to producing a caveat — the +// guard must not accidentally disable the split-mode signal entirely. +func TestOutstandingCaveat_GroupAlgoInFlight_FallsBackWhenRunningListEmpty(t *testing.T) { + st := proto.StatusReply{GroupAlgoInFlight: 1} // GroupAlgoRunning is nil/empty + got := outstandingCaveat(st, "mygroup") + if got == "" { + t.Fatal("outstandingCaveat = \"\", want a non-empty caveat when GroupAlgoRunning is empty and GroupAlgoInFlight > 0") + } + if !strings.Contains(got, "group-algo") { + t.Errorf("caveat = %q, want it to name group-algo", got) + } +} + // TestOutstandingCaveat_GroupAlgoInFlightZero_NoCaveat pins the other // direction for the same field: GroupAlgoInFlight==0 (the JSON zero value, // indistinguishable from "field never populated") must NOT, by itself, @@ -195,24 +238,15 @@ func TestOutstandingCaveat_AnalyticsPlusGroupAlgo_NamesBothWithOverlayClause(t * } } -// TestAttachOutstanding_NilClient_ReturnsEmpty: attachOutstanding must never -// panic or block on a nil client (e.g. --no-index / daemon-down paths that -// never dial) — it degrades to "no caveat", same as an absent RSS reading. -func TestAttachOutstanding_NilClient_ReturnsEmpty(t *testing.T) { - if got := attachOutstanding(nil, "mygroup"); got != "" { - t.Errorf("attachOutstanding(nil, ...) = %q, want empty", got) - } -} - // TestAttachOutstandingWith_QueriesAndAppliesCaveat is the pinning test for -// the attachOutstanding→outstandingCaveat BRIDGE (#6047 review round 2 -// finding 3): a mutation that replaces attachOutstanding's body with "call -// the fetcher, discard the result, return ”" left the entire ./internal/cli -// suite green, because nothing exercised the bridge — only the nil-client -// short-circuit and the pure outstandingCaveat function were pinned -// separately. This drives attachOutstandingWith with a fake fetcher standing -// in for c.Status, proving the fetched StatusReply is actually threaded -// through to the returned caveat. +// the fetch→outstandingCaveat BRIDGE (#6047 review round 2 finding 3, closed +// in round 3): a mutation that replaces attachOutstandingWith's body with +// "call the fetcher, discard the result, return ”" must die here — this is +// now the wizard's ONLY entry point into the caveat computation (production +// calls toIndexOutcome with c.Status, which has this exact statusFetcher +// signature). Drives attachOutstandingWith with a fake fetcher standing in +// for c.Status, proving the fetched StatusReply is actually threaded through +// to the returned caveat. func TestAttachOutstandingWith_QueriesAndAppliesCaveat(t *testing.T) { fetch := func() (proto.StatusReply, error) { return proto.StatusReply{GroupAlgoRunning: []string{"mygroup"}}, nil @@ -245,10 +279,51 @@ func TestAttachOutstandingWith_FetchError_ReturnsEmpty(t *testing.T) { } } -// TestAttachOutstandingWith_NilFetcher_ReturnsEmpty: a nil fetcher (mirrors -// attachOutstanding's nil-client short-circuit) must never panic. +// TestAttachOutstandingWith_NilFetcher_ReturnsEmpty: a nil fetcher must never +// panic — it degrades to "no caveat", same as an absent RSS reading. func TestAttachOutstandingWith_NilFetcher_ReturnsEmpty(t *testing.T) { if got := attachOutstandingWith(nil, "mygroup"); got != "" { t.Errorf("attachOutstandingWith(nil, ...) = %q, want empty", got) } } + +// TestToIndexOutcome_ThreadsCaveatFromFetcher is the REQUIRED regression for +// #6047 review round 3's "close the last seam" ask: toIndexOutcome +// (wizard_tui_run.go) now takes a statusFetcher directly (production passes +// c.Status) instead of a *client.Client, specifically so a mutation that +// bypasses attachOutstandingWith and hardcodes Outstanding: "" is caught +// here rather than only at the (now-deleted) trivial nil-client wrapper. +// This drives toIndexOutcome itself with a fake fetcher reporting an +// outstanding stage and asserts the caveat survives all the way to the +// returned wiztui.IndexOutcome. +func TestToIndexOutcome_ThreadsCaveatFromFetcher(t *testing.T) { + fetch := func() (proto.StatusReply, error) { + return proto.StatusReply{GroupAlgoRunning: []string{"mygroup"}}, nil + } + oc := toIndexOutcome(rebuildOutcome{entities: 10, rels: 20}, wiztui.InstallSummary{}, fetch, "mygroup") + if oc.Outstanding == "" { + t.Fatal("toIndexOutcome(...).Outstanding empty, want the fetcher's caveat threaded through") + } + if !strings.Contains(oc.Outstanding, "group-algo") { + t.Errorf("Outstanding = %q, want it to name group-algo", oc.Outstanding) + } +} + +// TestToIndexOutcome_ErrorOutcome_NeverQueriesStatus pins the other +// direction: an error outcome must return immediately with Outstanding "" +// and must NEVER invoke fetch — nothing was indexed, so there is nothing to +// ask the daemon about. +func TestToIndexOutcome_ErrorOutcome_NeverQueriesStatus(t *testing.T) { + calls := 0 + fetch := func() (proto.StatusReply, error) { + calls++ + return proto.StatusReply{GroupAlgoRunning: []string{"mygroup"}}, nil + } + oc := toIndexOutcome(rebuildOutcome{err: errors.New("boom")}, wiztui.InstallSummary{}, fetch, "mygroup") + if oc.Outstanding != "" { + t.Errorf("Outstanding = %q, want empty on an error outcome", oc.Outstanding) + } + if calls != 0 { + t.Errorf("fetch called %d times on an error outcome, want 0", calls) + } +} diff --git a/internal/cli/wizard_split_progress_test.go b/internal/cli/wizard_split_progress_test.go index 88693941f..e22ee5b9e 100644 --- a/internal/cli/wizard_split_progress_test.go +++ b/internal/cli/wizard_split_progress_test.go @@ -14,12 +14,23 @@ import ( "time" "github.com/cajasmota/grafel/internal/cli/wiztui" + "github.com/cajasmota/grafel/internal/daemon/proto" "github.com/cajasmota/grafel/internal/install/detect" "github.com/cajasmota/grafel/internal/progress" "github.com/cajasmota/grafel/internal/statusfile" "github.com/cajasmota/grafel/internal/testsupport" ) +// fakeIdleStatusFetch stands in for c.Status in these toIndexOutcome tests +// (#6047 review round 3 — toIndexOutcome takes a statusFetcher, not a +// *client.Client, precisely so tests exercise the same seam production +// does). It reports a fully idle StatusReply: these tests assert on +// Entities/Rels/Err, not on the completion-honesty caveat, so "nothing +// outstanding" is the correct, uninteresting answer here. +func fakeIdleStatusFetch() (proto.StatusReply, error) { + return proto.StatusReply{}, nil +} + // fakeSplitClock advances virtual time on Sleep so the poll loop's timeout // accounting works without any real delay. type fakeSplitClock struct { @@ -242,7 +253,7 @@ func TestSplit_OutcomeCarriesClassifiedStats(t *testing.T) { if o.err != nil { t.Fatalf("unexpected error: %v", o.err) } - oc := toIndexOutcome(o, wiztui.InstallSummary{}, nil, "") + oc := toIndexOutcome(o, wiztui.InstallSummary{}, fakeIdleStatusFetch, "grp") if oc.Entities != E || oc.Rels != R { t.Fatalf("IndexOutcome stats = (%d,%d); want (%d,%d)", oc.Entities, oc.Rels, E, R) } @@ -847,7 +858,7 @@ func TestRunSplitIndexCore_ThreadsPerRepoStatsIntoOutcome(t *testing.T) { t.Fatalf("len(o.repoStats) = %d, want 3", len(o.repoStats)) } - oc := toIndexOutcome(o, wiztui.InstallSummary{}, nil, "") + oc := toIndexOutcome(o, wiztui.InstallSummary{}, fakeIdleStatusFetch, "grp") if len(oc.RepoStats) != 3 { t.Fatalf("len(IndexOutcome.RepoStats) = %d, want 3", len(oc.RepoStats)) } diff --git a/internal/cli/wizard_tui_run.go b/internal/cli/wizard_tui_run.go index 7b6173621..b45f118b2 100644 --- a/internal/cli/wizard_tui_run.go +++ b/internal/cli/wizard_tui_run.go @@ -404,7 +404,7 @@ func streamIndexWithSummary(evCh chan<- progress.Event, outCh chan<- wiztui.Inde if !perRepoRows { o.repoStats = nil // monorepo: suppress the aggregate-as-repo-row overlay } - outCh <- toIndexOutcome(o, summary, c, group) + outCh <- toIndexOutcome(o, summary, c.Status, group) return } @@ -421,7 +421,7 @@ func streamIndexWithSummary(evCh chan<- progress.Event, outCh chan<- wiztui.Inde rpcCh := triggerRebuild(c, group, token) o := forwardBrokerToChannel(ctx, sseCh, rpcCh, evCh) cancel() - outCh <- toIndexOutcome(o, summary, c, group) + outCh <- toIndexOutcome(o, summary, c.Status, group) return } } @@ -429,7 +429,7 @@ func streamIndexWithSummary(evCh chan<- progress.Event, outCh chan<- wiztui.Inde // No dashboard broker — just trigger the rebuild and wait for the outcome. rpcCh := triggerRebuild(c, group, token) o := <-rpcCh - outCh <- toIndexOutcome(o, summary, c, group) + outCh <- toIndexOutcome(o, summary, c.Status, group) } // triggerRebuild fires the daemon Rebuild RPC for group on a goroutine and @@ -525,10 +525,19 @@ func jsonUnmarshalEvent(data string, e *progress.Event) bool { // toIndexOutcome maps a rebuildOutcome to the TUI's terminal IndexOutcome, // attaching the captured install summary so the Done screen renders it. On a // genuine success it also attaches the completion-honesty caveat (#6047) by -// querying the daemon's status ONE more time via c for group — best-effort, -// see attachOutstanding's doc; never attempted on an error outcome, since -// nothing was indexed to have an outstanding stage. -func toIndexOutcome(o rebuildOutcome, summary wiztui.InstallSummary, c *client.Client, group string) wiztui.IndexOutcome { +// querying the daemon's status ONE more time via fetch for group — +// best-effort, see attachOutstandingWith's doc; never attempted on an error +// outcome, since nothing was indexed to have an outstanding stage. +// +// fetch is a statusFetcher (production always passes c.Status, never a +// *client.Client directly) — #6047 review round 3: taking the client +// directly here let a mutation that bypassed attachOutstandingWith entirely +// (calling attachOutstanding's nil-check and stopping) still leave the whole +// suite green, since nothing exercised toIndexOutcome with anything other +// than a nil client. Taking the narrower statusFetcher type means a fake in +// wizard_split_progress_test.go's toIndexOutcome tests now exercises the +// exact same seam production does, closing that gap. +func toIndexOutcome(o rebuildOutcome, summary wiztui.InstallSummary, fetch statusFetcher, group string) wiztui.IndexOutcome { if o.err != nil { return wiztui.IndexOutcome{Err: o.err, Install: summary} } @@ -542,7 +551,7 @@ func toIndexOutcome(o rebuildOutcome, summary wiztui.InstallSummary, c *client.C Elapsed: elapsed, Install: summary, RepoStats: toWiztuiRepoStats(o.repoStats), - Outstanding: attachOutstanding(c, group), + Outstanding: attachOutstandingWith(fetch, group), } } diff --git a/internal/cli/wiztui/model.go b/internal/cli/wiztui/model.go index 5efc4f5b7..58519cd40 100644 --- a/internal/cli/wiztui/model.go +++ b/internal/cli/wiztui/model.go @@ -498,6 +498,17 @@ func (m Model) updateKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { // already-captured interim stats — the alternative to just waiting for // the final outcome to land on its own. if msg.String() == "enter" && m.idx.queryable && !m.idx.terminal { + // #6047 round 3, acknowledged not addressed: !m.idx.terminal here + // only reflects what THIS model has already processed off outCh — + // the real terminal outcome can already be sitting in outCh, + // unread, at the exact instant this keypress lands (waitOutcome's + // receive and this Update call are two separate tea.Msg + // deliveries). In that millisecond-scale window the background + // pass may have already acked for real, and this still prints the + // earlyFinishOutstandingCaveat below for work that just finished. + // Errs toward caution (a stale caveat, never a missing one) and + // is far narrower than the defect this file fixes, so it is left + // as a known, LOW-severity gap rather than chased here. m.idx.terminal = true // Overlay the interim classify's per-repo stats FIRST (so a repo // that emitted no progress events shows its real count, not 0),