From 90cf3f3f1fec7e73c299d63de49dc681613d0529 Mon Sep 17 00:00:00 2001 From: "Stephen R. van den Berg" Date: Wed, 30 Sep 2026 03:08:51 +0200 Subject: [PATCH] fix(cli): stop the diff formatter repainting every block when a run lands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A full-screen repaint draws every settled block, so when a background [cli].diff_formatter run landed and the re-render hook was keyless, the repaint dropped every diff block's cached rows and re-consulted the memo for each. Once the transcript held more diff keys than the memo, the repaint always re-missed at least one block, spawned a run for it, and that run landed and repainted again — an endless loop of formatter runs and full repaints. Carry the landed DiffKey on the hook and remember, per block, the keys its last render consulted, so a landed run drops only the blocks that asked for its key. Gate the drop on the memo still holding the key: re-rendering a block whose key was evicted would re-run the formatter and evict another live key, so an evicted block keeps the rows it already shows. The memo is raised to 128 so a normal transcript does not reach the eviction path, and keyed by a 128-bit hash of the diff text rather than the text itself, so an entry retains only the formatted output (~33-41% less retained heap). Also record the formatter's 1 MiB size cap as a fallback reason in the CLI reference. --- docs/CLI.md | 6 +- docs/CLI.zh-CN.md | 5 +- internal/frontend/termrender/diffformatter.go | 90 ++++++++++++++----- .../frontend/termrender/diffformatter_test.go | 2 +- internal/frontend/tui/diffformat_test.go | 65 +++++++++++--- internal/frontend/tui/program.go | 14 +-- internal/frontend/tui/render.go | 15 ---- internal/frontend/tui/screen.go | 35 ++++---- 8 files changed, 156 insertions(+), 76 deletions(-) diff --git a/docs/CLI.md b/docs/CLI.md index 20a50ae480..7340646cb5 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -155,9 +155,9 @@ In the full-screen TUI it runs off the render path: the built-in rows are drawn first and replaced once its output is ready, so a slow formatter cannot freeze the UI. -On any failure, timeout, or empty output the built-in renderer is kept. Like -`[cli].update_channel` it is user/global only — a project-local `reasonix.toml` -cannot set it. +On any failure, timeout, empty output, or a diff larger than 1 MiB, the +built-in renderer is kept. Like `[cli].update_channel` it is user/global only — +a project-local `reasonix.toml` cannot set it. `[agent].embedded_diff_detection = true` marks a shell result whose whole output is a unified diff — e.g. `bash` running `git diff` — so the CLI/TUI and desktop diff --git a/docs/CLI.zh-CN.md b/docs/CLI.zh-CN.md index b1b352e6b8..2c2cfd24ec 100644 --- a/docs/CLI.zh-CN.md +++ b/docs/CLI.zh-CN.md @@ -136,8 +136,9 @@ diff 卡片，以及整段输出就是 diff 的 shell 结果（见下方 全屏 TUI 中它在渲染路径之外运行：先画内置行，待其输出就绪后再替换，因此慢格式化器不会 卡住界面。 -命令失败、超时或输出为空时保留内置渲染器。与 `[cli].update_channel` 一样，它仅属于 -用户/全局配置，项目内的 `reasonix.toml` 无法设置。 +命令失败、超时、输出为空或 diff 超过 1 MiB 时保留内置渲染器。与 +`[cli].update_channel` 一样，它仅属于用户/全局配置，项目内的 `reasonix.toml` +无法设置。 `[agent].embedded_diff_detection = true` 会把整段输出为 unified diff 的 shell 结果 （例如 `bash` 运行 `git diff`）标记出来，让 CLI/TUI 与桌面端把它画成彩色 diff 而不是 diff --git a/internal/frontend/termrender/diffformatter.go b/internal/frontend/termrender/diffformatter.go index c334ba833e..86d8674884 100644 --- a/internal/frontend/termrender/diffformatter.go +++ b/internal/frontend/termrender/diffformatter.go @@ -9,6 +9,7 @@ package termrender import ( "bytes" "context" + "hash/maphash" "os" "os/exec" "strconv" @@ -49,8 +50,8 @@ func configureDiffFormatter(cfg *config.Config) { } diffFormatMu.Lock() activeDiffFormatter = argv - diffFormatCache, diffFormatOrder = map[diffFormatKey]diffFormatEntry{}, nil - diffFormatInflight = map[diffFormatKey]bool{} + diffFormatCache, diffFormatOrder = map[DiffKey]diffFormatEntry{}, nil + diffFormatInflight = map[DiffKey]bool{} diffFormatDisabled = false diffFormatMu.Unlock() } @@ -62,17 +63,18 @@ func SetDiffFormatterForTest(argv []string) (restore func()) { diffFormatMu.Lock() prev := activeDiffFormatter activeDiffFormatter = argv - diffFormatCache, diffFormatOrder = map[diffFormatKey]diffFormatEntry{}, nil - diffFormatInflight = map[diffFormatKey]bool{} + diffFormatCache, diffFormatOrder = map[DiffKey]diffFormatEntry{}, nil + diffFormatInflight = map[DiffKey]bool{} diffFormatMu.Unlock() return func() { SetDiffFormatterForTest(prev) } } // SetDiffFormatNotify registers fn as the re-render hook: the formatter calls it -// from its goroutine once a background run lands, so the frontend can repaint and -// pick up the result. Pass nil to run the formatter inline instead — a renderer -// that cannot re-render then still shows the output on the frame it draws. -func SetDiffFormatNotify(fn func()) { +// from its goroutine with the key that landed, so the frontend can repaint only +// the rows that asked for it. Pass nil to run the formatter inline instead — a +// renderer that cannot re-render then still shows the output on the frame it +// draws. +func SetDiffFormatNotify(fn func(DiffKey)) { diffFormatMu.Lock() diffFormatNotify = fn diffFormatMu.Unlock() @@ -184,11 +186,27 @@ func formatToolDiff(diff string, width int) ([]string, bool) { return rows, true } -const diffFormatCacheMax = 32 +const diffFormatCacheMax = 128 -type diffFormatKey struct { - content string - width int +// DiffKey identifies a memoized [cli].diff_formatter result: a 128-bit hash of +// the diff text and the width it was laid out to. Hashing rather than holding +// the text keeps the memo from keeping a second copy of a diff a block already +// carries, so a landed background run can still re-render only the rows that +// asked for its key without the memo paying for the input again. +type DiffKey struct { + Hash [2]uint64 + Width int +} + +// diffKeySeeds give the two hash halves; two seeds make a 128-bit key, so two +// distinct diffs collide with probability below 2^-115 at the memo's size. +var diffKeySeeds = [2]maphash.Seed{maphash.MakeSeed(), maphash.MakeSeed()} + +func diffKeyOf(content string, width int) DiffKey { + return DiffKey{ + Hash: [2]uint64{maphash.String(diffKeySeeds[0], content), maphash.String(diffKeySeeds[1], content)}, + Width: width, + } } type diffFormatEntry struct { @@ -198,20 +216,47 @@ type diffFormatEntry struct { var ( diffFormatMu sync.Mutex - diffFormatCache = map[diffFormatKey]diffFormatEntry{} - diffFormatOrder []diffFormatKey - diffFormatInflight = map[diffFormatKey]bool{} + diffFormatCache = map[DiffKey]diffFormatEntry{} + diffFormatOrder []DiffKey + diffFormatInflight = map[DiffKey]bool{} // diffFormatDisabled latches once a run hits diffFormatTimeout. A formatter // that hangs once hangs again, and each retry costs a full timeout — worst on // a resize, which changes the width key and so misses the per-(content,width) // cache. Once latched the built-in renderer is used. diffFormatDisabled bool // diffFormatNotify, when set, makes the run asynchronous and is called from - // the formatter goroutine once a result lands, so the frontend repaints and - // picks it up. Nil runs the formatter inline. - diffFormatNotify func() + // the formatter goroutine with the landed key once a result arrives, so the + // frontend repaints and picks it up. Nil runs the formatter inline. + diffFormatNotify func(DiffKey) + // renderKeySink, when non-nil, receives every key a render consults. Set only + // for the duration of a RenderKeys call, on the one goroutine that renders. + renderKeySink func(DiffKey) ) +// RenderKeys runs render and returns its output with every formatter key it +// consulted, so the caller can remember which keys its rows depend on. The keys +// come back whether the memo served them or a background run was started. +func RenderKeys(render func() string) (string, []DiffKey) { + var keys []DiffKey + prev := renderKeySink + renderKeySink = func(k DiffKey) { keys = append(keys, k) } + out := render() + renderKeySink = prev + return out, keys +} + +// HasDiffFormat reports whether the memo still holds a result for key. A +// frontend that scopes its repaint by key checks this before dropping a block's +// rows: re-rendering a block whose key the memo has since evicted would re-run +// the formatter and evict another live key, a cascade that never settles, so the +// evicted block keeps the rows it already shows. +func HasDiffFormat(key DiffKey) bool { + diffFormatMu.Lock() + defer diffFormatMu.Unlock() + _, ok := diffFormatCache[key] + return ok +} + // formatDiffCached returns the formatter's output for (diff, width), memoized // per key. With a re-render hook set (SetDiffFormatNotify) the run goes to a // background goroutine and this returns ok=false at once, so the caller draws @@ -221,7 +266,10 @@ func formatDiffCached(diff string, width int) (string, bool) { if len(activeDiffFormatter) == 0 { return "", false } - key := diffFormatKey{content: diff, width: width} + key := diffKeyOf(diff, width) + if renderKeySink != nil { + renderKeySink(key) + } diffFormatMu.Lock() if diffFormatDisabled { diffFormatMu.Unlock() @@ -254,7 +302,7 @@ func formatDiffCached(diff string, width int) (string, bool) { notify := diffFormatNotify diffFormatMu.Unlock() if ok && notify != nil { - notify() + notify(key) } }() return "", false @@ -272,7 +320,7 @@ func formatDiffCached(diff string, width int) (string, bool) { // storeDiffFormat records a result under key, evicting the oldest entry when the // memo is full. Callers hold diffFormatMu. -func storeDiffFormat(key diffFormatKey, text string, ok bool) { +func storeDiffFormat(key DiffKey, text string, ok bool) { if _, seen := diffFormatCache[key]; seen { return } diff --git a/internal/frontend/termrender/diffformatter_test.go b/internal/frontend/termrender/diffformatter_test.go index 1d741685b5..0b2a3e1769 100644 --- a/internal/frontend/termrender/diffformatter_test.go +++ b/internal/frontend/termrender/diffformatter_test.go @@ -319,7 +319,7 @@ func TestFormatDiffCachedAsyncWhenNotifyHookSet(t *testing.T) { setDiffFormatter([]string{"cat"}) done := make(chan struct{}, 1) - SetDiffFormatNotify(func() { done <- struct{}{} }) + SetDiffFormatNotify(func(DiffKey) { done <- struct{}{} }) defer SetDiffFormatNotify(nil) if _, ok := formatDiffCached("body", 80); ok { diff --git a/internal/frontend/tui/diffformat_test.go b/internal/frontend/tui/diffformat_test.go index 7d39b605e5..221d23f659 100644 --- a/internal/frontend/tui/diffformat_test.go +++ b/internal/frontend/tui/diffformat_test.go @@ -1,6 +1,7 @@ package tui import ( + "fmt" "runtime" "strings" "testing" @@ -14,15 +15,15 @@ import ( // diffFormatHarness installs a stdin→stdout formatter (cat) and a re-render // hook, so a formatter run is asynchronous exactly as the full-screen TUI makes -// it. The returned channel receives one signal per landed run. -func diffFormatHarness(t *testing.T) (landed <-chan struct{}) { +// it. The returned channel receives the key of each landed run. +func diffFormatHarness(t *testing.T) (landed <-chan termrender.DiffKey) { t.Helper() if runtime.GOOS == "windows" { t.Skip("no portable stdin→stdout filter") } restore := termrender.SetDiffFormatterForTest([]string{"cat"}) - ch := make(chan struct{}, 8) - termrender.SetDiffFormatNotify(func() { ch <- struct{}{} }) + ch := make(chan termrender.DiffKey, 256) + termrender.SetDiffFormatNotify(func(k termrender.DiffKey) { ch <- k }) t.Cleanup(func() { termrender.SetDiffFormatNotify(nil) restore() @@ -30,13 +31,16 @@ func diffFormatHarness(t *testing.T) (landed <-chan struct{}) { return ch } -// waitLanded fails unless a formatter run lands within the deadline. -func waitLanded(t *testing.T, landed <-chan struct{}) { +// waitLanded fails unless a formatter run lands within the deadline, and returns +// the key it landed for. +func waitLanded(t *testing.T, landed <-chan termrender.DiffKey) termrender.DiffKey { t.Helper() select { - case <-landed: + case k := <-landed: + return k case <-time.After(5 * time.Second): t.Fatal("formatter run never landed") + return termrender.DiffKey{} } } @@ -76,9 +80,9 @@ func TestLandedDiffResultReplacesTheSettledShellDiffRows(t *testing.T) { if !strings.Contains(before, "+1 -1") { t.Fatalf("built-in rows not drawn while the run was in flight:\n%s", before) } - waitLanded(t, landed) + key := waitLanded(t, landed) - m.Update(diffFormattedMsg{}) + m.Update(diffFormattedMsg{key: key}) after := strings.Join(cachedRows(m), "\n") if strings.Contains(after, "+1 -1") { @@ -101,9 +105,9 @@ func TestLandedDiffResultReplacesTheSettledWriteCardRows(t *testing.T) { if !strings.Contains(before, "1 - OLD_LINE") { t.Fatalf("built-in card body not drawn while the run was in flight:\n%s", before) } - waitLanded(t, landed) + key := waitLanded(t, landed) - m.Update(diffFormattedMsg{}) + m.Update(diffFormattedMsg{key: key}) after := strings.Join(cachedRows(m), "\n") if strings.Contains(after, "1 - OLD_LINE") { @@ -158,8 +162,8 @@ func TestFormattedShellDiffRowsFitTheTranscriptWidth(t *testing.T) { apply(m, eventwire.Event{Kind: "tool_dispatch", Tool: &eventwire.Tool{ID: "t1", Name: "bash", Args: `{"command":"git diff"}`}}) apply(m, eventwire.Event{Kind: "tool_result", Tool: &eventwire.Tool{ID: "t1", Name: "bash", Output: diff, OutputDiff: true}}) _ = cachedRows(m) - waitLanded(t, landed) - m.Update(diffFormattedMsg{}) + key := waitLanded(t, landed) + m.Update(diffFormattedMsg{key: key}) cw := m.contentWidth() for _, l := range rawRows(m) { @@ -168,3 +172,38 @@ func TestFormattedShellDiffRowsFitTheTranscriptWidth(t *testing.T) { } } } + +// A transcript holding more diff keys than the memo must still settle: each +// landed run is fed back the way the program feeds it, and a run may only be +// answered by repainting the block that asked for its key, so a repaint cannot +// spawn runs for blocks it never touched. Keyless, every landed run dropped +// every diff block and a transcript past the memo size never converged. +func TestManyDiffBlocksSettleRatherThanLoop(t *testing.T) { + const n = 160 // more diff keys than diffFormatCacheMax + landed := diffFormatHarness(t) + m, _ := testModel(t) + for i := range n { + id := fmt.Sprintf("t%d", i) + diff := fmt.Sprintf("diff --git a/f%d.go b/f%d.go\n--- a/f%d.go\n+++ b/f%d.go\n@@ -1 +1 @@\n-old%d\n+new%d\n", i, i, i, i, i, i) + apply(m, + eventwire.Event{Kind: "tool_dispatch", Tool: &eventwire.Tool{ID: id, Name: "bash", Args: `{"command":"git diff"}`}}, + eventwire.Event{Kind: "tool_result", Tool: &eventwire.Tool{ID: id, Name: "bash", Output: diff, OutputDiff: true}}, + ) + } + _ = cachedRows(m) // first render: every distinct block starts one run + + answered := 0 + for { + select { + case k := <-landed: + m.Update(diffFormattedMsg{key: k}) + _ = cachedRows(m) + answered++ + if answered > 4*n { + t.Fatalf("the landed-run repaint keeps spawning runs (%d answered)", answered) + } + case <-time.After(time.Second): + return // quiesced: no run landed for a second + } + } +} diff --git a/internal/frontend/tui/program.go b/internal/frontend/tui/program.go index b808e90449..7057cc496e 100644 --- a/internal/frontend/tui/program.go +++ b/internal/frontend/tui/program.go @@ -59,7 +59,7 @@ func Run(ctx context.Context, opts Options) error { // path: built-in rows now, formatted ones when the run lands. Inline writes to // the scrollback, where a row cannot be repainted, so it stays inline. if !opts.Inline { - termrender.SetDiffFormatNotify(func() { p.Send(diffFormattedMsg{}) }) + termrender.SetDiffFormatNotify(func(k termrender.DiffKey) { p.Send(diffFormattedMsg{key: k}) }) defer termrender.SetDiffFormatNotify(nil) } _, err := p.Run() @@ -147,9 +147,11 @@ type ( } statusTickMsg struct{} // diffFormattedMsg is the diff formatter's background run reporting a - // result; the model repaints on it so the formatted rows replace the - // built-in ones drawn while the run was in flight. - diffFormattedMsg struct{} + // result; the model repaints the rows that asked for that key so the + // formatted rows replace the built-in ones drawn while the run was in flight. + diffFormattedMsg struct { + key termrender.DiffKey + } ) func newModel(ctx context.Context, opts Options) *model { @@ -295,7 +297,7 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case statusTickMsg: return m, tea.Batch(m.fetchStatus(), tickStatus()) case diffFormattedMsg: - m.invalidateDiffBlocks() + m.invalidateDiffKey(msg.key) return m, m.commit() case actionMsg: if msg.err != nil { @@ -452,7 +454,7 @@ func (m *model) settledChunk(it *Item) (settledPrint, bool) { row := *it p := settledPrint{render: func(w int, hideRail bool) string { return withThought(&row, shown, w, renderSayPart(row.Text[shown:end], shown == 0, w, hideRail)) - }, diff: drawsThroughDiffFormatter(it)} + }} if m.scr != nil && shown == 0 && row.Reasoning != "" { row.Fold = foldShut p.row = &row diff --git a/internal/frontend/tui/render.go b/internal/frontend/tui/render.go index e0cbb37f38..69a1ecfb8a 100644 --- a/internal/frontend/tui/render.go +++ b/internal/frontend/tui/render.go @@ -214,21 +214,6 @@ func diffRows(out string, width int, f outputFold) []string { return rows } -// drawsThroughDiffFormatter reports a row whose settled render may consult the -// configured [cli].diff_formatter: a tool card with a diff body, or an answer -// carrying a ```diff / ```patch fence. A block drawn from one must drop its -// cached rows when a background run lands, or it keeps the built-in rows it -// showed while the run was in flight. -func drawsThroughDiffFormatter(it *Item) bool { - switch it.Kind { - case ItemTool: - return it.Tool != nil && (it.Tool.Diff != "" || it.Tool.OutputDiff) - case ItemSay: - return strings.Contains(it.Text, "```diff") || strings.Contains(it.Text, "```patch") - } - return false -} - // renderUsage is what one model request cost, under a quiet rule: history, // so it stays in the scrollback in a quieter voice than the footer. func renderUsage(u *eventwire.Usage, width int) string { diff --git a/internal/frontend/tui/screen.go b/internal/frontend/tui/screen.go index 60ce2b7557..a1ec7ae48a 100644 --- a/internal/frontend/tui/screen.go +++ b/internal/frontend/tui/screen.go @@ -48,9 +48,8 @@ type block struct { hideRail bool cells ansi.Method lines []string - // diff marks a block whose rows draw through the diff formatter, so a - // landed background run must drop its cached lines. - diff bool + // keys are the diff-formatter keys the block's last render consulted. + keys []termrender.DiffKey // row is the settled row the block draws, when it draws one: a shell // call's output opens and shuts through it. row *Item @@ -60,7 +59,9 @@ func (b *block) at(width int, hideRail bool) []string { cells := termrender.Cells() if b.lines == nil || b.width != width || b.hideRail != hideRail || b.cells != cells { b.width, b.hideRail, b.cells = width, hideRail, cells - b.lines = wrapLines(b.render(width, hideRail), width) + out, keys := termrender.RenderKeys(func() string { return b.render(width, hideRail) }) + b.keys = keys + b.lines = wrapLines(out, width) } return b.lines } @@ -116,8 +117,6 @@ func wrapLines(out string, width int) []string { type settledPrint struct { render func(width int, hideRail bool) string row *Item - // diff marks a print whose rows draw through the diff formatter. - diff bool } // settledRow keeps a copy of the row to draw from. Full screen, a shell @@ -126,7 +125,7 @@ func (m *model) settledRow(row Item, shown int) settledPrint { if m.scr != nil && (row.Kind == ItemTool || row.Kind == ItemSay && row.Reasoning != "" && shown == 0) { row.Fold = foldShut } - return settledPrint{render: func(w int, hideRail bool) string { return renderItem(&row, w, shown, hideRail) }, row: &row, diff: drawsThroughDiffFormatter(&row)} + return settledPrint{render: func(w int, hideRail bool) string { return renderItem(&row, w, shown, hideRail) }, row: &row} } // publish sends what settled where this screen keeps it: blocks of the full @@ -134,7 +133,7 @@ func (m *model) settledRow(row Item, shown int) settledPrint { func (m *model) publish(out []settledPrint) tea.Cmd { if m.scr != nil { for _, p := range out { - m.scr.blocks = append(m.scr.blocks, block{render: p.render, row: p.row, diff: p.diff}) + m.scr.blocks = append(m.scr.blocks, block{render: p.render, row: p.row}) } return nil } @@ -151,21 +150,27 @@ func (m *model) emit(render func(int, bool) string) tea.Cmd { return m.publish([]settledPrint{{render: render}}) } -// invalidateDiffBlocks drops the cached rows of every block that draws through -// the diff formatter. A background run reports its result long after the block -// that asked for it was drawn and its rows cached, so without this the built-in -// rows it showed while the run was in flight would stay on screen. -func (m *model) invalidateDiffBlocks() { - if m.scr == nil { +// invalidateDiffKey drops the cached rows of the blocks whose last render +// consulted key, but only while the memo still holds it. Dropping every diff +// block would re-render — and so re-spawn — blocks that never asked for it; +// dropping an evicted key's block would re-run the formatter and evict another +// live key, a cascade that never settles, so that block keeps its rows. +func (m *model) invalidateDiffKey(key termrender.DiffKey) { + if m.scr == nil || !termrender.HasDiffFormat(key) { return } for i := range m.scr.blocks { - if m.scr.blocks[i].diff { + if m.scr.blocks[i].hasKey(key) { m.scr.blocks[i].lines = nil } } } +// hasKey reports whether the block's last render consulted key. +func (b *block) hasKey(key termrender.DiffKey) bool { + return slices.Contains(b.keys, key) +} + // fillScreen pushes whatever the terminal shows into its scrollback before a // burst of prints. The renderer places a print by scrolling it in above a // frame it takes to sit at the bottom of the screen; on a screen not yet full