fix(#6047): wizard stops reporting Done while the pipeline is still running; scope the memory readout - #6051
Merged
Merged
Conversation
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
…al, 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:<group>` 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
… 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The index wizard printed
Doneand exited whileanalytics,group-algoandlinkswere still running — the stages that produce communities, pagerank, centrality and cross-repo links.grafel statuscarried the caveat; the wizard carried nothing.Measured on a full greenfield index of a large group. The wizard reported
Done · 15m55swith 427,303 entities. Eighteen minutes past the recorded index timestamp the gate was still held (stage_gate: holder=links:<group>), so roughly ten further minutes of pipeline work ran behind a screen that saidDone. The counts shown next to it —cross-repo edges,flows, and the whole overlay — were provisional.Same defect class as the release-blocker batch: completion reported before completion.
Instant
Done, plus what is still runningThe wizard does not block on the gate. The graph genuinely is queryable at that point, and blocking would have turned a 16-minute wait into a 26-minute one for no user gain. It keeps its instant
Doneand names the outstanding stages, reusing the exact phraseprintAnnotationStatusalready prints sostatusand the wizard cannot drift apart. Nothing outstanding prints nothing extra.The finish-early path was the bug, not an edge case
The first implementation excluded the "press enter to finish now" path on the grounds that its
queryableBanneralready explained the situation. It does not: the banner is what is on screen before enter, andindexview.godispatches onterminalbeforequeryable, sodoneSummary()replaces it. Review probe on the original:A user in a hurry landed on a screen strictly less honest than the one they left — #6047's own defect, on the path most likely to be taken.
queryable && !terminalmeans the background pass has not acked, so the caveat is unconditional there; pinned both ways, including that pressing enter after a clean terminal outcome does not manufacture one.The memory readout is scoped, not inflated
0.9 GBwas the engine's instantaneous RSS at exit, positioned to read as the run's total. Measured againstpsacross 399 samples of that same run:It now tracks a high-water mark and is labelled
engine peak. Scoping rather than aggregating is deliberate: an unlabelled0.9 GBwas ambiguous, but a barepeak 1.4 GBwould have been an explicit claim about the run's maximum that is wrong by 2.5x. The extract children cannot be added yet — their self-reported RSS isruntime.MemStats.Sys, which excludes the CGO/tree-sitter heap that dominates those processes (#6048). The doc comment records the measurements and why. Verified the high-water resets per run vianewIndexView.Reading the right half of the status reply
The first implementation's doc had split and monolith backwards.
PendingAlgo/PendingLinks/GroupAlgoRunningare populated only unders.scheduler != nil— the monolith branch — so in split mode, the default and the mode this bug was measured in,outstandingCaveatcollapsed to four stage-gate string comparisons and those three fields contributed nothing.It now also consumes
GroupAlgoInFlight, which split mode does populate from the sidecar, and checksStageGateBargingforanalytics— a stage named in the issue's own evidence line and previously unread. Analytics is a quality-metrics history scan (cmd/grafel/rebuild_history.go:81), unrelated to the overlay, so when it is the sole outstanding stage the overlay clause is suppressed and only the bare "the graph itself is queryable" remains. Pinned in both directions.GroupAlgoInFlightis process-wide, not group-scoped, so it fires only when the preciseGroupAlgoRunninglist is empty. Without that guard, an operator indexing group B made the wizard report group-algo outstanding for group A, which had fully drained — the direction the issue explicitly forbids. Review probe:Both over- and under-correction are pinned: removing the guard fails, and removing the fallback fails too, so split mode still speaks.
Verification
go build ./... && go vet ./... && gofmt -l .clean;internal/cliandinternal/cli/wiztuigreen at raw exit 0 viartk proxy.14 mutations, all producing real test failures. Two independent review rounds; findings 1, 2 and 4 verified closed by execution, and the mutation that survived round 2 now dies. That one is worth recording: hardcoding
Outstanding: ""intoIndexOutcomeoriginally left the wholeinternal/clisuite green, because the daemon query was reachable only through an adapter tested for its nil case.toIndexOutcomenow takes astatusFetcherrather than*client.Client, so test fakes traverse the identical signature production does — the residual is which function value is bound at three call sites, which the compiler type-checks.Known and disclosed
All pre-existing, none introduced here, all documented in-source:
indexstate.GroupAlgoBegin()fires on execution (scheduler.go:2391) and on gate-deferral (scheduler.go:2222), never at timer arming — so a pass armed but not yet at the gate is invisible. Verified rather than assumed; the signal covers more than first claimed, and the doc was corrected upward.Donewith no caveat (5s heartbeat)."". Unknown should say so.Status()— a wedged daemon hangs the completion screen after 16 minutes of work.outChwhen the keypress lands, so the constant may caveat work that just finished. Errs toward caution; source-commented so the next reader does not rediscover it as a bug.Independently adversarially reviewed across three rounds.
Closes #6047