fix: retry LSP/MCP toolsets after tool calls, covering env-wrapped commands (fixes #2457) by simonferquel-clanker · Pull Request #2458 · docker/docker-agent

simonferquel-clanker · 2026-04-17T13:13:35Z

Summary

When an MCP server is configured but not available at session start, the agent now retries within the same user turn — after each tool-call batch — without requiring a new user message.

Root cause and fix

MCP double-watcher race (B1)

Toolset.Start() previously spawned go watchConnection(...) unconditionally. With the reprobe loop calling Toolset.Start() during reconnect backoff (when ts.started==false but the goroutine is alive), a second watcher goroutine could be spawned, causing racing doStart() calls and unsafe close/recreate of ts.restarted.

Fix: added watcherAlive bool to Toolset. Start() only spawns the watcher when !watcherAlive; the goroutine clears the flag on all exit paths via defer.

Reprobe after each tool-call batch

Added reprobe() called after every tool-call batch in the runtime loop. It re-runs ensureToolSetsAreStarted() without emitting MCPInitStarted/Finished events (no TUI spinner flicker). If new tools appear (by name-set diff), it emits a ToolsetInfo update immediately. The updated tool list is picked up by the next iteration's getTools() — so the model sees new tools within the same user turn, without a new user message.

Deduplicated warnings + recovery notices

StartableToolSet tracks failure streaks with ShouldReportFailure() (emits exactly one warning per streak) and ConsumeRecovery() (emits exactly one "now available" warning when a toolset recovers). Both surface via WarningEvent → notification.WarningCmd() — persistent TUI notifications that stay until dismissed.

Changes

File	What changed
`pkg/tools/mcp/mcp.go`	`watcherAlive` flag; gated watcher spawn; `watchConnection` clears flag on exit
`pkg/tools/startable.go`	`ShouldReportFailure()`, `ConsumeRecovery()`, failure-streak tracking fields
`pkg/agent/agent.go`	`ensureToolSetsAreStarted` uses dedup methods; `DrainWarnings` includes recovery notices
`pkg/runtime/loop.go`	`reprobe()` after each tool-call batch; `emitLifecycleEvents` param on `getTools()`
`pkg/runtime/event.go`	`WarningEvent` used for both failures and recovery
`pkg/tui/page/chat/runtime_events.go`	`WarningEvent` → `notification.WarningCmd()`

Tests

pkg/tools/startable_test.go: ShouldReportFailure/ConsumeRecovery behaviour (one warning per streak, recovery fires once)
pkg/agent/agent_test.go: TestAgentReProbeEmitsWarningThenNotice, TestAgentNoDuplicateStartWarnings
pkg/runtime/runtime_test.go: TestReprobe_NewToolsAvailableAfterToolCall, TestReprobe_NoChangeMeansNoExtraEvents

tmux e2e (MCP, validated locally)

Single user turn: agent runs touch /flag → reprobe fires → next model response uses new MCP tool natively.

✓ Shell  touch /tmp/e2e-test/mcp-ready
✓ hello  name=World  Hello, World! MCP is working.
Tools sidebar: 6 tools available  ← was 5

rumpl · 2026-04-17T13:19:58Z

I'm not sure I understand the fix, isn't the lsp tooset startable? Wouldn't re-starting the lsp toolset be enough?

simonferquel-clanker · 2026-04-17T13:20:33Z

Good question — the LSP toolset is Startable, but there are two separate problems that a naive re-start doesn't cover:

1. env -C ./backend gopls looks like a successful start

When the command is env -C ./backend gopls, exec.Command("env", ...).Start() always returns nil — env is always present. The fact that env then immediately runs gopls, finds it missing, and exits 127 is invisible to cmd.Start(). By the time we return from LSPTool.Start() / startLocked(), h.cmd has been set and the call returned nil, so StartableToolSet sets started = true and considers the toolset live.

A subsequent call to Start() hits the if s.started { return nil } short-circuit and never retries — even though the underlying process died in milliseconds.

2. The HealthChecker interface closes that gap

The fix adds a HealthChecker interface (Healthy() bool) that StartableToolSet checks on every Start() call even when started == true. LSPTool.Healthy() returns h.cmd != nil.

The new watchExit goroutine is the mechanism that transitions h.cmd from non-nil to nil: it is the sole owner of cmd.Wait(), blocks until the process exits, then resets h.cmd = nil. So after env gopls exits 127, Healthy() returns false, StartableToolSet resets started = false, and the next Start() call actually retries — which will succeed once gopls has been installed.

3. The reprobe loop ties it together within a single turn

Even with the above, without the reprobe the agent would only see new tools at the start of the next user turn (when getTools() is called at the top of the run loop). The reprobe() call after each tool-call batch re-runs ensureToolSetsAreStarted immediately, so if a shell tool call installed gopls, the very next model response in the same turn already has the LSP tools in its available-tool list — no new user message needed.

simonferquel

With #2460 about to get merged, the env gopls thing is moot. This PR should not try to cover this pattern anymore

simonferquel-clanker · 2026-04-20T10:52:30Z

Done — removed in commit 0c65de91.

Removed (everything that was specifically for the env -C wrapper pattern):

HealthChecker interface and the health-check block in StartableToolSet.Start()
LSPTool.Healthy(), watchExit goroutine, dead atomic.Bool, exited chan
stopAndWait reverted to stopLocked with direct cmd.Wait()
Toolset.Healthy() on the MCP side (no longer needed without the interface)
All related tests (TestLSPTool_Healthy_*, TestLSPTool_StartableToolSet_RetriesAfterProcessExits, TestStartableToolSet_HealthChecker_*)

Retained (the actual fix from this PR):

watcherAlive flag in MCP Toolset + gated watcher spawn (B1: prevents double watchConnection goroutine on reprobe)
reprobe() in the runtime loop (fires after each tool-call batch, no MCPInit* spinner noise)
ShouldReportFailure / ConsumeRecovery deduplication and recovery notices
All agent-level and runtime-level reprobe tests

krissetto · 2026-04-20T11:07:59Z

use the proper notification component when surfacing things to the TUI. A warning one in this case might be appropriate

simonferquel-clanker · 2026-04-20T11:11:17Z

Done in commit ad1b1734.

Dropped the separate NoticeEvent/InfoCmd path entirely. Both start-failure warnings and toolset recovery messages now go through the existing WarningEvent → notification.WarningCmd() path.

The key distinction: TypeInfo notifications auto-hide after 3 seconds; TypeWarning notifications are persistent and stay until the user dismisses them — which is the right behaviour for toolset availability state changes that the user should not miss.

Changes:

NoticeEvent, Notice(), formatToolNotice removed from runtime
DrainNotices, addToolNotice, pendingNotices removed from agent
ConsumeRecovery() now calls addToolWarning("X is now available")
emitAgentWarnings() simplified back to draining warnings only
NoticeEvent TUI handler and "notice" client type registration removed

simonferquel · 2026-04-20T11:28:20Z


 	// Cancel the process-lifetime context to stop the readNotifications
-	// goroutine and (if the process didn't exit cleanly) kill the process.
+	// goroutine and force-kill the process if it hasn't exited cleanly.


NIT: revert that comment only change

Done — lsp.go reverted to upstream/main in commit 65d42382. Both lsp.go and lsp_test.go are now identical to upstream.

simonferquel · 2026-04-20T11:28:51Z

 	t.Parallel()

 	tool := NewLSPTool("gopls", nil, nil, "/tmp")
-	tools, err := tool.Tools(t.Context())


all these files changes should be reverted

Done — lsp_test.go fully reverted to upstream/main in commit 65d42382. No changes from this PR remain in pkg/tools/builtin/.

simonferquel · 2026-04-20T11:29:50Z

-	// Use WithoutCancel so the watcher outlives the caller's context;
-	// the only way to stop it is via Stop() setting ts.stopping.
-	go ts.watchConnection(context.WithoutCancel(ctx))
+	// Spawn the connection watcher only when no watcher is already running.


is this file changes still needed with the env -C concern removal ?

Yes — this change is independent of the env -C concern and is still needed.

The watcherAlive flag fixes a race introduced by the reprobe loop itself:

watchConnection sets ts.started = false at the top of its reconnect iteration (before sleeping its backoff), while the goroutine is still alive.

If reprobe() fires during that window, StartableToolSet.Start() sees started == false and calls Toolset.Start().

Without the guard, Toolset.Start() would unconditionally spawn a second go watchConnection(...), resulting in two goroutines racing to call doStart() and unsafely close/recreate ts.restarted.

The fix: Toolset.Start() only spawns watchConnection when !watcherAlive. The goroutine clears the flag on all exit paths via defer. While an existing watcher is mid-backoff, reprobe skips the spawn. Once the watcher truly gives up, the next reprobe starts a fresh one.

simonferquel · 2026-04-20T11:33:05Z


+// WorkingDir returns the working directory of the LSP server process.
+// This is intended for testing only.
+func (t *LSPTool) WorkingDir() string {


if it is test only, it should be in lsp_test.go

After rebasing onto upstream/main (which now includes PR #2460 that added WorkingDir()), our branch has no diff in lsp.go or lsp_test.go — both files are identical to upstream. Nothing to move.

… turn When an MCP server is configured but unavailable at session start, the agent now retries automatically after every tool-call batch — within the same user turn — without requiring a new user message. ## Core changes ### MCP double-watcher race (pkg/tools/mcp/mcp.go) Added watcherAlive bool to Toolset. Toolset.Start() only spawns go watchConnection(...) when !watcherAlive. The goroutine clears the flag on all exit paths via defer. This prevents reprobe() from spawning a second watcher while an existing one is mid-backoff (ts.started==false but goroutine alive), which would cause racing doStart() calls and unsafe close/recreate of ts.restarted. ### Failure deduplication + recovery notices (pkg/tools/startable.go) ShouldReportFailure() returns true exactly once per failure streak, suppressing repeated 'start failed' warnings on every retry. ConsumeRecovery() returns true exactly once when a previously-failed toolset successfully starts, triggering a 'now available' warning. Both surface via WarningEvent -> notification.WarningCmd() (persistent TUI notifications that stay until dismissed). ### Reprobe after each tool-call batch (pkg/runtime/loop.go) reprobe() is called after every tool-call batch. It re-runs ensureToolSetsAreStarted() without emitting MCPInitStarted/Finished events (no TUI spinner flicker), emits any pending warnings, and emits a ToolsetInfo event when new tools appear. The updated tool list is picked up by the top-of-loop getTools() on the next iteration, so the model sees new tools in its very next response within the same user turn. ### TUI (pkg/agent/agent.go, pkg/runtime/loop.go, pkg/runtime/event.go) DrainWarnings() now includes both failure and recovery messages. WarningEvent used for all toolset lifecycle notifications. ## Tests - pkg/tools/startable_test.go: ShouldReportFailure/ConsumeRecovery behaviour (one warning per streak, recovery fires once, Stop resets) - pkg/agent/agent_test.go: TestAgentReProbeEmitsWarningThenNotice, TestAgentNoDuplicateStartWarnings - pkg/runtime/runtime_test.go: TestReprobe_NewToolsAvailableAfterToolCall, TestReprobe_NoChangeMeansNoExtraEvents Fixes docker#2457 Assisted-By: docker-agent

simonferquel

Approving my own agent feels not enough. cc @krissetto for another approve :)

simonferquel-clanker requested a review from a team as a code owner April 17, 2026 13:13

simonferquel requested changes Apr 20, 2026

View reviewed changes

simonferquel self-requested a review April 20, 2026 11:00

simonferquel reviewed Apr 20, 2026

View reviewed changes

simonferquel-clanker force-pushed the fix/retry-missing-lsp-mcp-2457 branch from 65d4238 to 0176f95 Compare April 20, 2026 11:37

simonferquel approved these changes Apr 20, 2026

View reviewed changes

dgageot approved these changes Apr 20, 2026

View reviewed changes

simonferquel merged commit f41413d into docker:main Apr 20, 2026
5 checks passed

BrewTestBot mentioned this pull request Apr 20, 2026

docker-agent 1.48.0 Homebrew/homebrew-core#278471

Open

Conversation

simonferquel-clanker commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Root cause and fix

MCP double-watcher race (B1)

Reprobe after each tool-call batch

Deduplicated warnings + recovery notices

Changes

Tests

tmux e2e (MCP, validated locally)

Uh oh!

rumpl commented Apr 17, 2026

Uh oh!

simonferquel-clanker commented Apr 17, 2026

Uh oh!

simonferquel left a comment

Choose a reason for hiding this comment

Uh oh!

simonferquel-clanker commented Apr 20, 2026

Uh oh!

krissetto commented Apr 20, 2026

Uh oh!

simonferquel-clanker commented Apr 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonferquel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

simonferquel-clanker commented Apr 17, 2026 •

edited

Loading