Stabilize activity detection on restore and startup#182
Conversation
| if !tab.lastUserInputAt.IsZero() && now.Sub(tab.lastUserInputAt) <= localInputEchoSuppressWindow { | ||
| // Suppress local-echo candidates and keep pending so the next flush | ||
| // cycle can re-evaluate once the echo window has passed. | ||
| tab.pendingVisibleOutput = true | ||
| return "", 0, false |
There was a problem hiding this comment.
🟡 Local echo suppression path does not update activityDigest, causing false-positive activity detection
When the local-input echo suppression window is active in noteVisibleActivityLocked, the code returns early without saving the computed activityDigest. This means that after the 500ms echo window passes, the next call to noteVisibleActivityLocked (triggered by any new output, even non-visible control sequences) will compare the current screen digest against the stale/uninitialized activityDigest and see changed = true, falsely counting the echoed typing as real agent activity.
Root Cause and Trace
Step-by-step reproduction:
- Agent produces output →
activityDigest = D_agent,activityDigestInit = true - User types a character →
lastUserInputAtis set viarecordLocalInputEchoWindowatmodel_input_echo.go:46 - Terminal echoes the character → screen changes →
pendingVisibleOutput = true - Flush fires →
noteVisibleActivityLockedatmodel_activity_visibility.go:46-50sees the echo window is active. It skips savingactivityDigest(which still holdsD_agent) and keepspendingVisibleOutput = true. - Echo window passes (500ms). Non-visible output arrives (e.g. control sequences like cursor repositioning).
- New flush fires →
noteVisibleActivityLockedcomputes digestD_echo(screen with echo char).changed = (D_echo != D_agent)→ true, even though the only actual screen change was the suppressed echo. - Code falls through to
model_activity_visibility.go:52-55, setstab.lastVisibleOutput = now→ false positive activity indicator for ~2 seconds.
Contrast with the bootstrap suppression path at model_activity_visibility.go:38-44, which correctly updates activityDigest and activityDigestInit so re-evaluation won't re-trigger on the same screen content.
Impact: A brief (~2-second) false activity flash in the tab/dashboard UI after the user types, if non-visible control output arrives just after the 500ms echo window expires.
| if !tab.lastUserInputAt.IsZero() && now.Sub(tab.lastUserInputAt) <= localInputEchoSuppressWindow { | |
| // Suppress local-echo candidates and keep pending so the next flush | |
| // cycle can re-evaluate once the echo window has passed. | |
| tab.pendingVisibleOutput = true | |
| return "", 0, false | |
| if !tab.lastUserInputAt.IsZero() && now.Sub(tab.lastUserInputAt) <= localInputEchoSuppressWindow { | |
| // Suppress local-echo candidates and keep pending so the next flush | |
| // cycle can re-evaluate once the echo window has passed. | |
| tab.activityDigest = digest | |
| tab.activityDigestInit = true | |
| tab.pendingVisibleOutput = true | |
| return "", 0, false | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| if tagSessionName != "" { | ||
| opts := m.getTmuxOptions() | ||
| sessionName := tagSessionName | ||
| timestamp := strconv.FormatInt(tagTimestamp, 10) | ||
| go func() { | ||
| _ = tmux.SetSessionTagValue(sessionName, tmux.TagLastOutputAt, timestamp, opts) | ||
| }() | ||
| } |
There was a problem hiding this comment.
🔴 Data race: getTmuxOptions() called from tab actor goroutine without synchronization
The new tabEventWriteOutput handler in the tab actor goroutine calls m.getTmuxOptions() at internal/ui/center/tab_actor.go:386, which reads m.tmuxConfig.ServerName and m.tmuxConfig.ConfigPath without any lock. Meanwhile, SetTmuxConfig (internal/ui/center/model.go:76-78) writes those same fields from the main update goroutine (called via app_input_messages_dialogs.go:182).
Before this PR, the tab actor never called getTmuxOptions() — confirmed by checking the prior version of tab_actor.go. This is a new concurrent access introduced by this change.
Root Cause and Impact
getTmuxOptions() reads two string fields on m.tmuxConfig:
func (m *Model) getTmuxOptions() tmux.Options {
opts := tmux.DefaultOptions()
if m.tmuxConfig.ServerName != "" {
opts.ServerName = m.tmuxConfig.ServerName
}
...
}SetTmuxConfig writes them from the main goroutine:
func (m *Model) SetTmuxConfig(serverName, configPath string) {
m.tmuxConfig.ServerName = serverName
m.tmuxConfig.ConfigPath = configPath
...
}Under the Go memory model, concurrent read/write of a string (which is a two-word struct: pointer + length) is a data race that can produce a corrupted value (e.g., mismatched pointer/length). In practice, this could cause a crash or pass garbage to tmux commands. The race is most likely triggered when a user changes tmux server settings while agents are actively producing output.
Prompt for agents
In internal/ui/center/tab_actor.go, the tabEventWriteOutput case (around line 385-392) calls m.getTmuxOptions() from the tab actor goroutine, which races with SetTmuxConfig on the main goroutine. To fix this, capture the tmux options on the main update goroutine side (in updatePTYFlush in model_input_lifecycle_pty.go) and pass them through the tabEvent struct alongside hasMoreBuffered and visibleSeq. Add a new field like `tmuxOpts tmux.Options` to the tabEvent struct in tab_actor.go, populate it when constructing the event in updatePTYFlush (around line 225-232), and use ev.tmuxOpts instead of m.getTmuxOptions() at line 386 in tab_actor.go.
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
Describe the change and intended behavior.
Quality Checklist
make devchecklocally.make lint-strict-newlocally for changed code.make harness-presets.go test ./internal/tmux ./internal/e2e.