fix(agent): bound startup alert buffering
- commit
- 7018e79e81e603e454956487acc52a37a62d63b1
- author
- brice · · 2026-08-04 08:59 UTC
- parent
- 149c6c3
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
5 files · +39 -6
M packages/agent/cli.go +6 -1
| @@ -32,6 +32,9 @@ import ( | |||
| 32 | 32 | // manager call back into the Interactive instance built later in | |
| 33 | 33 | // runInteractive. Alerts are buffered until the instance is attached so | |
| 34 | 34 | // an extension that signals attention during startup is not silently lost. | |
| 35 | + | // Keep the startup buffer bounded because extensions can flood their host. | |
| 36 | + | const maxBufferedInteractiveAlerts = 64 | |
| 37 | + | ||
| 35 | 38 | type interactiveExtHooks struct { | |
| 36 | 39 | mu sync.Mutex | |
| 37 | 40 | interactive *modes.Interactive | |
| @@ -77,7 +80,9 @@ func (h *interactiveExtHooks) Alert(extName string, alert extproto.AlertRequest) | |||
| 77 | 80 | } | |
| 78 | 81 | h.mu.Lock() | |
| 79 | 82 | if h.interactive == nil { | |
| 80 | - | h.pendingAlerts = append(h.pendingAlerts, interactiveAlert{extName: extName, alert: alert}) | |
| 83 | + | if len(h.pendingAlerts) < maxBufferedInteractiveAlerts { | |
| 84 | + | h.pendingAlerts = append(h.pendingAlerts, interactiveAlert{extName: extName, alert: alert}) | |
| 85 | + | } | |
| 81 | 86 | h.mu.Unlock() | |
| 82 | 87 | return | |
| 83 | 88 | } | |
| @@ -32,6 +32,9 @@ import ( | |||||
| 32 | // manager call back into the Interactive instance built later in | 32 | // manager call back into the Interactive instance built later in | ||
| 33 | // runInteractive. Alerts are buffered until the instance is attached so | 33 | // runInteractive. Alerts are buffered until the instance is attached so | ||
| 34 | // an extension that signals attention during startup is not silently lost. | 34 | // an extension that signals attention during startup is not silently lost. | ||
| 35 | + | // Keep the startup buffer bounded because extensions can flood their host. | |||
| 36 | + | const maxBufferedInteractiveAlerts = 64 | |||
| 37 | + | ||||
| 35 | type interactiveExtHooks struct { | 38 | type interactiveExtHooks struct { | ||
| 36 | mu sync.Mutex | 39 | mu sync.Mutex | ||
| 37 | interactive *modes.Interactive | 40 | interactive *modes.Interactive | ||
| @@ -77,7 +80,9 @@ func (h *interactiveExtHooks) Alert(extName string, alert extproto.AlertRequest) | |||||
| 77 | } | 80 | } | ||
| 78 | h.mu.Lock() | 81 | h.mu.Lock() | ||
| 79 | if h.interactive == nil { | 82 | if h.interactive == nil { | ||
| 80 | - | h.pendingAlerts = append(h.pendingAlerts, interactiveAlert{extName: extName, alert: alert}) | 83 | + | if len(h.pendingAlerts) < maxBufferedInteractiveAlerts { |
| 84 | + | h.pendingAlerts = append(h.pendingAlerts, interactiveAlert{extName: extName, alert: alert}) | |||
| 85 | + | } | |||
| 81 | h.mu.Unlock() | 86 | h.mu.Unlock() | ||
| 82 | return | 87 | return | ||
| 83 | } | 88 | } | ||
M packages/agent/cli_alert_test.go +5 -2
| @@ -10,12 +10,15 @@ import ( | |||
| 10 | 10 | func TestInteractiveExtHooksBufferStartupAlerts(t *testing.T) { | |
| 11 | 11 | hooks := &interactiveExtHooks{} | |
| 12 | 12 | hooks.Alert("question-ext", extproto.AlertRequest{Kind: extproto.AlertKindBell, Reason: "question_ready"}) | |
| 13 | + | for n := 0; n < maxBufferedInteractiveAlerts+8; n++ { | |
| 14 | + | hooks.Alert("question-ext", extproto.AlertRequest{Kind: extproto.AlertKindBell, Reason: "flood"}) | |
| 15 | + | } | |
| 13 | 16 | ||
| 14 | 17 | hooks.mu.Lock() | |
| 15 | 18 | pending := len(hooks.pendingAlerts) | |
| 16 | 19 | hooks.mu.Unlock() | |
| 17 | - | if pending != 1 { | |
| 18 | - | t.Fatalf("pending startup alerts = %d, want 1", pending) | |
| 20 | + | if pending != maxBufferedInteractiveAlerts { | |
| 21 | + | t.Fatalf("pending startup alerts = %d, want bounded capacity %d", pending, maxBufferedInteractiveAlerts) | |
| 19 | 22 | } | |
| 20 | 23 | ||
| 21 | 24 | iv := &modes.Interactive{} | |
| @@ -10,12 +10,15 @@ import ( | |||||
| 10 | func TestInteractiveExtHooksBufferStartupAlerts(t *testing.T) { | 10 | func TestInteractiveExtHooksBufferStartupAlerts(t *testing.T) { | ||
| 11 | hooks := &interactiveExtHooks{} | 11 | hooks := &interactiveExtHooks{} | ||
| 12 | hooks.Alert("question-ext", extproto.AlertRequest{Kind: extproto.AlertKindBell, Reason: "question_ready"}) | 12 | hooks.Alert("question-ext", extproto.AlertRequest{Kind: extproto.AlertKindBell, Reason: "question_ready"}) | ||
| 13 | + | for n := 0; n < maxBufferedInteractiveAlerts+8; n++ { | |||
| 14 | + | hooks.Alert("question-ext", extproto.AlertRequest{Kind: extproto.AlertKindBell, Reason: "flood"}) | |||
| 15 | + | } | |||
| 13 | 16 | ||||
| 14 | hooks.mu.Lock() | 17 | hooks.mu.Lock() | ||
| 15 | pending := len(hooks.pendingAlerts) | 18 | pending := len(hooks.pendingAlerts) | ||
| 16 | hooks.mu.Unlock() | 19 | hooks.mu.Unlock() | ||
| 17 | - | if pending != 1 { | 20 | + | if pending != maxBufferedInteractiveAlerts { |
| 18 | - | t.Fatalf("pending startup alerts = %d, want 1", pending) | 21 | + | t.Fatalf("pending startup alerts = %d, want bounded capacity %d", pending, maxBufferedInteractiveAlerts) |
| 19 | } | 22 | } | ||
| 20 | 23 | ||||
| 21 | iv := &modes.Interactive{} | 24 | iv := &modes.Interactive{} | ||
M packages/agent/ext/ext_test.go +13 -2
| @@ -222,7 +222,19 @@ func TestOpenPanelEmitsCorrectFrame(t *testing.T) { | |||
| 222 | 222 | ||
| 223 | 223 | func TestAlertEmitsStructuredFrame(t *testing.T) { | |
| 224 | 224 | h := newHarness("alert-ext") | |
| 225 | - | go h.ext.Run() | |
| 225 | + | runDone := make(chan error, 1) | |
| 226 | + | go func() { runDone <- h.ext.Run() }() | |
| 227 | + | t.Cleanup(func() { | |
| 228 | + | _ = h.hostW.Close() | |
| 229 | + | select { | |
| 230 | + | case err := <-runDone: | |
| 231 | + | if err != nil { | |
| 232 | + | t.Errorf("extension Run: %v", err) | |
| 233 | + | } | |
| 234 | + | case <-time.After(2 * time.Second): | |
| 235 | + | t.Error("extension Run did not exit after closing host input") | |
| 236 | + | } | |
| 237 | + | }) | |
| 226 | 238 | h.handshake(t) | |
| 227 | 239 | ||
| 228 | 240 | go h.ext.Alert(AlertRequest{Kind: AlertKindBell, Reason: "question_ready"}) | |
| @@ -235,7 +247,6 @@ func TestAlertEmitsStructuredFrame(t *testing.T) { | |||
| 235 | 247 | if alert.Kind != AlertKindBell || alert.Reason != "question_ready" { | |
| 236 | 248 | t.Fatalf("alert = %+v, want bell/question_ready", alert) | |
| 237 | 249 | } | |
| 238 | - | h.hostW.Close() | |
| 239 | 250 | } | |
| 240 | 251 | ||
| 241 | 252 | func TestToolConfirmationRequestedEvent(t *testing.T) { | |
| @@ -222,7 +222,19 @@ func TestOpenPanelEmitsCorrectFrame(t *testing.T) { | |||||
| 222 | 222 | ||||
| 223 | func TestAlertEmitsStructuredFrame(t *testing.T) { | 223 | func TestAlertEmitsStructuredFrame(t *testing.T) { | ||
| 224 | h := newHarness("alert-ext") | 224 | h := newHarness("alert-ext") | ||
| 225 | - | go h.ext.Run() | 225 | + | runDone := make(chan error, 1) |
| 226 | + | go func() { runDone <- h.ext.Run() }() | |||
| 227 | + | t.Cleanup(func() { | |||
| 228 | + | _ = h.hostW.Close() | |||
| 229 | + | select { | |||
| 230 | + | case err := <-runDone: | |||
| 231 | + | if err != nil { | |||
| 232 | + | t.Errorf("extension Run: %v", err) | |||
| 233 | + | } | |||
| 234 | + | case <-time.After(2 * time.Second): | |||
| 235 | + | t.Error("extension Run did not exit after closing host input") | |||
| 236 | + | } | |||
| 237 | + | }) | |||
| 226 | h.handshake(t) | 238 | h.handshake(t) | ||
| 227 | 239 | ||||
| 228 | go h.ext.Alert(AlertRequest{Kind: AlertKindBell, Reason: "question_ready"}) | 240 | go h.ext.Alert(AlertRequest{Kind: AlertKindBell, Reason: "question_ready"}) | ||
| @@ -235,7 +247,6 @@ func TestAlertEmitsStructuredFrame(t *testing.T) { | |||||
| 235 | if alert.Kind != AlertKindBell || alert.Reason != "question_ready" { | 247 | if alert.Kind != AlertKindBell || alert.Reason != "question_ready" { | ||
| 236 | t.Fatalf("alert = %+v, want bell/question_ready", alert) | 248 | t.Fatalf("alert = %+v, want bell/question_ready", alert) | ||
| 237 | } | 249 | } | ||
| 238 | - | h.hostW.Close() | |||
| 239 | } | 250 | } | ||
| 240 | 251 | ||||
| 241 | func TestToolConfirmationRequestedEvent(t *testing.T) { | 252 | func TestToolConfirmationRequestedEvent(t *testing.T) { | ||
M packages/agent/modes/alert_test.go +10 -0
| @@ -89,27 +89,37 @@ func TestCancelledMainTurnDoesNotBell(t *testing.T) { | |||
| 89 | 89 | i.startTurn(context.Background(), "question") | |
| 90 | 90 | ||
| 91 | 91 | deadline := time.Now().Add(time.Second) | |
| 92 | + | started := false | |
| 92 | 93 | for time.Now().Before(deadline) { | |
| 93 | 94 | i.mu.Lock() | |
| 94 | 95 | busy := i.busy | |
| 95 | 96 | i.mu.Unlock() | |
| 96 | 97 | if busy { | |
| 98 | + | started = true | |
| 97 | 99 | break | |
| 98 | 100 | } | |
| 99 | 101 | time.Sleep(time.Millisecond) | |
| 100 | 102 | } | |
| 103 | + | if !started { | |
| 104 | + | t.Fatal("turn did not enter busy state") | |
| 105 | + | } | |
| 101 | 106 | i.CancelTurn() | |
| 102 | 107 | ||
| 103 | 108 | deadline = time.Now().Add(time.Second) | |
| 109 | + | stopped := false | |
| 104 | 110 | for time.Now().Before(deadline) { | |
| 105 | 111 | i.mu.Lock() | |
| 106 | 112 | busy := i.busy | |
| 107 | 113 | i.mu.Unlock() | |
| 108 | 114 | if !busy { | |
| 115 | + | stopped = true | |
| 109 | 116 | break | |
| 110 | 117 | } | |
| 111 | 118 | time.Sleep(time.Millisecond) | |
| 112 | 119 | } | |
| 120 | + | if !stopped { | |
| 121 | + | t.Fatal("turn did not finish after cancellation") | |
| 122 | + | } | |
| 113 | 123 | if got := term.String(); got != "" { | |
| 114 | 124 | t.Fatalf("cancelled main turn alert output = %q, want empty", got) | |
| 115 | 125 | } | |
| @@ -89,27 +89,37 @@ func TestCancelledMainTurnDoesNotBell(t *testing.T) { | |||||
| 89 | i.startTurn(context.Background(), "question") | 89 | i.startTurn(context.Background(), "question") | ||
| 90 | 90 | ||||
| 91 | deadline := time.Now().Add(time.Second) | 91 | deadline := time.Now().Add(time.Second) | ||
| 92 | + | started := false | |||
| 92 | for time.Now().Before(deadline) { | 93 | for time.Now().Before(deadline) { | ||
| 93 | i.mu.Lock() | 94 | i.mu.Lock() | ||
| 94 | busy := i.busy | 95 | busy := i.busy | ||
| 95 | i.mu.Unlock() | 96 | i.mu.Unlock() | ||
| 96 | if busy { | 97 | if busy { | ||
| 98 | + | started = true | |||
| 97 | break | 99 | break | ||
| 98 | } | 100 | } | ||
| 99 | time.Sleep(time.Millisecond) | 101 | time.Sleep(time.Millisecond) | ||
| 100 | } | 102 | } | ||
| 103 | + | if !started { | |||
| 104 | + | t.Fatal("turn did not enter busy state") | |||
| 105 | + | } | |||
| 101 | i.CancelTurn() | 106 | i.CancelTurn() | ||
| 102 | 107 | ||||
| 103 | deadline = time.Now().Add(time.Second) | 108 | deadline = time.Now().Add(time.Second) | ||
| 109 | + | stopped := false | |||
| 104 | for time.Now().Before(deadline) { | 110 | for time.Now().Before(deadline) { | ||
| 105 | i.mu.Lock() | 111 | i.mu.Lock() | ||
| 106 | busy := i.busy | 112 | busy := i.busy | ||
| 107 | i.mu.Unlock() | 113 | i.mu.Unlock() | ||
| 108 | if !busy { | 114 | if !busy { | ||
| 115 | + | stopped = true | |||
| 109 | break | 116 | break | ||
| 110 | } | 117 | } | ||
| 111 | time.Sleep(time.Millisecond) | 118 | time.Sleep(time.Millisecond) | ||
| 112 | } | 119 | } | ||
| 120 | + | if !stopped { | |||
| 121 | + | t.Fatal("turn did not finish after cancellation") | |||
| 122 | + | } | |||
| 113 | if got := term.String(); got != "" { | 123 | if got := term.String(); got != "" { | ||
| 114 | t.Fatalf("cancelled main turn alert output = %q, want empty", got) | 124 | t.Fatalf("cancelled main turn alert output = %q, want empty", got) | ||
| 115 | } | 125 | } | ||
M packages/agent/rpc.go +5 -1
| @@ -171,6 +171,8 @@ func (h *rpcExtHooks) OpenPanel(string, extproto.PanelSpec) {} | |||
| 171 | 171 | func (h *rpcExtHooks) UpdatePanel(string, string, string, []string, string) {} | |
| 172 | 172 | func (h *rpcExtHooks) ClosePanel(string, string) {} | |
| 173 | 173 | ||
| 174 | + | const maxPendingRPCExtEvents = 64 | |
| 175 | + | ||
| 174 | 176 | type rpcServer struct { | |
| 175 | 177 | ctx context.Context | |
| 176 | 178 | args Args | |
| @@ -486,7 +488,9 @@ func (s *rpcServer) writeExtensionEvent(payload map[string]any) { | |||
| 486 | 488 | s.writeMu.Lock() | |
| 487 | 489 | defer s.writeMu.Unlock() | |
| 488 | 490 | if !s.started || !s.authed { | |
| 489 | - | s.pendingExtEvents = append(s.pendingExtEvents, b) | |
| 491 | + | if len(s.pendingExtEvents) < maxPendingRPCExtEvents { | |
| 492 | + | s.pendingExtEvents = append(s.pendingExtEvents, b) | |
| 493 | + | } | |
| 490 | 494 | return | |
| 491 | 495 | } | |
| 492 | 496 | s.writeLocked(b) | |
| @@ -171,6 +171,8 @@ func (h *rpcExtHooks) OpenPanel(string, extproto.PanelSpec) {} | |||||
| 171 | func (h *rpcExtHooks) UpdatePanel(string, string, string, []string, string) {} | 171 | func (h *rpcExtHooks) UpdatePanel(string, string, string, []string, string) {} | ||
| 172 | func (h *rpcExtHooks) ClosePanel(string, string) {} | 172 | func (h *rpcExtHooks) ClosePanel(string, string) {} | ||
| 173 | 173 | ||||
| 174 | + | const maxPendingRPCExtEvents = 64 | |||
| 175 | + | ||||
| 174 | type rpcServer struct { | 176 | type rpcServer struct { | ||
| 175 | ctx context.Context | 177 | ctx context.Context | ||
| 176 | args Args | 178 | args Args | ||
| @@ -486,7 +488,9 @@ func (s *rpcServer) writeExtensionEvent(payload map[string]any) { | |||||
| 486 | s.writeMu.Lock() | 488 | s.writeMu.Lock() | ||
| 487 | defer s.writeMu.Unlock() | 489 | defer s.writeMu.Unlock() | ||
| 488 | if !s.started || !s.authed { | 490 | if !s.started || !s.authed { | ||
| 489 | - | s.pendingExtEvents = append(s.pendingExtEvents, b) | 491 | + | if len(s.pendingExtEvents) < maxPendingRPCExtEvents { |
| 492 | + | s.pendingExtEvents = append(s.pendingExtEvents, b) | |||
| 493 | + | } | |||
| 490 | return | 494 | return | ||
| 491 | } | 495 | } | ||
| 492 | s.writeLocked(b) | 496 | s.writeLocked(b) | ||