Skip to content

Commit 6ac5d16

Browse files
hugodutkahugodutka
authored
fix: agent message flickering (#12)
* fix: agent message flickering
1 parent faabad7 commit 6ac5d16

File tree

3 files changed

+92
-26
lines changed

3 files changed

+92
-26
lines changed

‎lib/screentracker/conversation.go

Lines changed: 19 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,17 @@ func (c *Conversation) StartSnapshotLoop(ctx context.Context) {
117117
case <-ctx.Done():
118118
return
119119
case <-time.After(c.cfg.SnapshotInterval):
120-
c.AddSnapshot(c.cfg.AgentIO.ReadScreen())
120+
// It's important that we hold the lock while reading the screen.
121+
// There's a race condition that occurs without it:
122+
// 1. The screen is read
123+
// 2. Independently, SendMessage is called and takes the lock.
124+
// 3. AddSnapshot is called and waits on the lock.
125+
// 4. SendMessage modifies the terminal state, releases the lock
126+
// 5. AddSnapshot adds a snapshot from a stale screen
127+
c.lock.Lock()
128+
screen := c.cfg.AgentIO.ReadScreen()
129+
c.addSnapshotInner(screen)
130+
c.lock.Unlock()
121131
}
122132
}
123133
}()
@@ -191,32 +201,21 @@ func (c *Conversation) updateLastAgentMessage(screen string, timestamp time.Time
191201
c.messages[len(c.messages)-1].Id = len(c.messages) - 1
192202
}
193203

194-
// This is a temporary hack to work around a bug in Claude Code 0.2.70.
195-
// https://github.com/anthropics/claude-code/issues/803
196-
// 0.2.71 should not need it anymore. We will remove it a couple of days
197-
// after the new version is released.
198-
func removeDuplicateClaude_0_2_70_Output(screen string) string {
199-
// this hack will only work if the terminal emulator is exactly 80 characters wide
200-
// this is hard-coded right now in the termexec package
201-
idx := strings.LastIndex(screen, "╭────────────────────────────────────────────╮ \n│ ✻ Welcome to Claude Code research preview! │")
202-
if idx == -1 {
203-
return screen
204+
// assumes the caller holds the lock
205+
func (c *Conversation) addSnapshotInner(screen string) {
206+
snapshot := screenSnapshot{
207+
timestamp: c.cfg.GetTime(),
208+
screen: screen,
204209
}
205-
return screen[idx:]
210+
c.snapshotBuffer.Add(snapshot)
211+
c.updateLastAgentMessage(screen, snapshot.timestamp)
206212
}
207213

208214
func (c *Conversation) AddSnapshot(screen string) {
209215
c.lock.Lock()
210216
defer c.lock.Unlock()
211217

212-
screen = removeDuplicateClaude_0_2_70_Output(screen)
213-
214-
snapshot := screenSnapshot{
215-
timestamp: c.cfg.GetTime(),
216-
screen: screen,
217-
}
218-
c.snapshotBuffer.Add(snapshot)
219-
c.updateLastAgentMessage(screen, snapshot.timestamp)
218+
c.addSnapshotInner(screen)
220219
}
221220

222221
type MessagePart interface {

‎lib/termexec/termexec.go

Lines changed: 61 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,17 +7,21 @@ import (
77
"log/slog"
88
"os"
99
"os/exec"
10+
"sync"
1011
"syscall"
1112
"time"
1213

1314
"github.com/ActiveState/termtest/xpty"
1415
"github.com/coder/agentapi/lib/logctx"
16+
"github.com/coder/agentapi/lib/util"
1517
"golang.org/x/xerrors"
1618
)
1719

1820
type Process struct {
19-
xp *xpty.Xpty
20-
execCmd *exec.Cmd
21+
xp *xpty.Xpty
22+
execCmd *exec.Cmd
23+
screenUpdateLock sync.RWMutex
24+
lastScreenUpdate time.Time
2125
}
2226

2327
type StartProcessConfig struct {
@@ -42,11 +46,38 @@ func StartProcess(ctx context.Context, args StartProcessConfig) (*Process, error
4246
return nil, err
4347
}
4448

49+
process := &Process{xp: xp, execCmd: execCmd}
50+
4551
go func() {
52+
// HACK: Working around xpty concurrency limitations
53+
//
54+
// Problem:
55+
// 1. We need to track when the terminal screen was last updated (for ReadScreen)
56+
// 2. xpty only updates terminal state through xp.ReadRune()
57+
// 3. xp.ReadRune() has a bug - it panics when SetReadDeadline is used
58+
// 4. Without deadlines, ReadRune blocks until the process outputs data
59+
//
60+
// Why this matters:
61+
// If we wrapped ReadRune + lastScreenUpdate in a mutex, this goroutine would
62+
// hold the lock while waiting for process output. Since ReadRune blocks indefinitely,
63+
// ReadScreen callers would be locked out until new output arrives. Even worse,
64+
// after output arrives, this goroutine could immediately reacquire the lock
65+
// for the next ReadRune call, potentially starving ReadScreen callers indefinitely.
66+
//
67+
// Solution:
68+
// Instead of using xp.ReadRune(), we directly use its internal components:
69+
// - pp.ReadRune() - handles the blocking read from the process
70+
// - xp.Term.WriteRune() - updates the terminal state
71+
//
72+
// This lets us apply the mutex only around the terminal update and timestamp,
73+
// keeping reads non-blocking while maintaining thread safety.
74+
//
75+
// Warning: This depends on xpty internals and may break if xpty changes.
76+
// A proper fix would require forking xpty or getting upstream changes.
77+
pp := util.GetUnexportedField(xp, "pp").(*xpty.PassthroughPipe)
4678
for {
47-
// calling ReadRune updates the terminal state. without it,
48-
// xp.State will always return an empty string
49-
if _, _, err := xp.ReadRune(); err != nil {
79+
r, _, err := pp.ReadRune()
80+
if err != nil {
5081
if err != io.EOF {
5182
logger.Error("Error reading from pseudo terminal", "error", err)
5283
}
@@ -55,18 +86,42 @@ func StartProcess(ctx context.Context, args StartProcessConfig) (*Process, error
5586
// unresponsive.
5687
return
5788
}
89+
process.screenUpdateLock.Lock()
90+
// writing to the terminal updates its state. without it,
91+
// xp.State will always return an empty string
92+
xp.Term.WriteRune(r)
93+
process.lastScreenUpdate = time.Now()
94+
process.screenUpdateLock.Unlock()
5895
}
5996
}()
6097

61-
return &Process{xp: xp, execCmd: execCmd}, nil
98+
return process, nil
6299
}
63100

64101
func (p *Process) Signal(sig os.Signal) error {
65102
return p.execCmd.Process.Signal(sig)
66103
}
67104

68105
// ReadScreen returns the contents of the terminal window.
106+
// It waits for the terminal to be stable for 16ms before
107+
// returning, or 48 ms since it's called, whichever is sooner.
108+
//
109+
// This logic acts as a kind of vsync. Agents regularly redraw
110+
// parts of the screen. If we naively snapshotted the screen,
111+
// we'd often capture it while it's being updated. This would
112+
// result in a malformed agent message being returned to the
113+
// user.
69114
func (p *Process) ReadScreen() string {
115+
for range 3 {
116+
p.screenUpdateLock.RLock()
117+
if time.Since(p.lastScreenUpdate) >= 16*time.Millisecond {
118+
state := p.xp.State.String()
119+
p.screenUpdateLock.RUnlock()
120+
return state
121+
}
122+
p.screenUpdateLock.RUnlock()
123+
time.Sleep(16 * time.Millisecond)
124+
}
70125
return p.xp.State.String()
71126
}
72127

‎lib/util/unsafe.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
package util
2+
3+
import (
4+
"reflect"
5+
"unsafe"
6+
)
7+
8+
// Based on https://stackoverflow.com/a/60598827
9+
func GetUnexportedField[T any](obj *T, fieldName string) any {
10+
field := reflect.ValueOf(obj).Elem().FieldByName(fieldName)
11+
return reflect.NewAt(field.Type(), unsafe.Pointer(field.UnsafeAddr())).Elem().Interface()
12+
}

0 commit comments

Comments
 (0)