Skip to content

aitools: raise the plugin probe timeout from 5s to 20s #7472

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
anton-107 merged 1 commit into from
Nov 12, 2026
Merged

aitools: raise the plugin probe timeout from 5s to 20s #7472

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .nextchanges/cli/aitools-plugin-probe-timeout.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* `databricks aitools install` and `databricks aitools update` now wait up to 20 seconds instead of 5 for a coding agent's CLI to answer the plugin capability check, so a slow first start is less likely to report the agent as unavailable. ([#7472](https://github.com/databricks/cli/pull/7472))
5 changes: 3 additions & 2 deletions libs/aitools/agents/detect.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@ import (
var lookPath = exec.LookPath

// pluginProbeTimeout bounds the `<agent> plugin --help` capability probe so a
// hung agent CLI cannot stall install.
const pluginProbeTimeout = 5 * time.Second
// hung agent CLI cannot stall install. It matches the probe timeout in
// libs/aitools/installer, which leaves headroom for a slow agent CLI cold start.
const pluginProbeTimeout = 20 * time.Second

// DisplayState describes how an agent should be presented in the install
// picker. It combines the two cheap presence signals (binary on PATH and
Expand Down
8 changes: 7 additions & 1 deletion libs/aitools/installer/plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,13 @@ var lookPath = exec.LookPath

const (
// pluginProbeTimeout bounds the `<agent> plugin --help` capability check.
pluginProbeTimeout = 5 * time.Second
// Agent CLIs are large Node/Rust binaries whose cold start (first run,
// antivirus scan, slow container disk) can take several seconds, and a probe
// that times out reports the agent as unavailable, so leave headroom.
// It also bounds `<agent> plugin marketplace list` in marketplaceRegistered,
// where a timeout means the marketplace is assumed already registered, so
// install does not record it and uninstall leaves it registered.
pluginProbeTimeout = 20 * time.Second

@simonfaltum simonfaltum Nov 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nice to have] libs/aitools/agents/detect.go:20 still has its own pluginProbeTimeout = 5 * time.Second for the same <agent> plugin --help probe in (*Agent).ProbePlugin. After this PR, two constants with the same name disagree (5s vs 20s). Only tests call ProbePlugin today. Its doc, though, says it is "meant to be called once, for a selected agent, right before install", so wiring it in later would bring back the 5s false "unavailable" result this PR fixes. Please either raise that constant too, or delete ProbePlugin with its constant and tests so only one probe is left.

@anton-107 anton-107 Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raised pluginProbeTimeout in libs/aitools/agents/detect.go to 20s as well, with a comment saying it matches the installer probe, so ProbePlugin no longer disagrees if it gets wired in. I kept ProbePlugin rather than deleting it, to keep this PR to the timeout change. (410f869)

Comment on lines 22 to +29

@simonfaltum simonfaltum Nov 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] This comment explains 20s by the cold start of plugin --help. The constant also bounds plugin marketplace list (line 124), where a timeout doesn't mark the agent unavailable. Instead marketplaceRegistered returns true, install doesn't record InstalledMarketplace, and a later uninstall leaves the marketplace registered. Anyone who tunes the probe timeout from this comment would change that behavior without knowing it. Please mention the second use here, or give marketplace list its own constant.

@anton-107 anton-107 Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment now also covers plugin marketplace list in marketplaceRegistered: on a timeout the marketplace is assumed already registered, so install does not record it and uninstall leaves it registered. (410f869)

@simonfaltum simonfaltum Nov 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nice to have] runAgentCmd (lines 66-70) calls process.Background without process.WithProcessGroup(). That leaves Cmd.WaitDelay at zero, and only the direct child is killed when this timeout fires. On Windows, an npm-installed Codex or Copilot runs as codex.cmd → cmd.exe → node → the native binary, with inherited stdio. If plugin --help hangs, the grandchild keeps the stdout/stderr pipes open and Wait blocks past 20s, possibly indefinitely, so "blocks for up to 20s" doesn't hold there. This predates the PR. Since #7475 already touches runAgentCmd, consider passing process.WithProcessGroup() there, as libs/localenv/uv.go does. It sets WaitDelay and signals the whole process group.

@anton-107 anton-107 Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed it's worth doing, but I'd keep it out of this PR, which only changes the timeout value. Two reasons:

  • aitools: typed fetch, state and agent CLI timeout errors #7475 rewrites runAgentCmd, including this exact process.Background call (it wraps the result in a timeout error). The option belongs there, or in a follow-up on top of it, so the stack doesn't edit the same line twice.
  • It changes behavior on both platforms, so it deserves its own test. On Unix the timeout would send SIGTERM to the whole group and allow a 10s grace period before SIGKILL, so a hung probe could take up to 30s instead of 20s. On Windows, WithProcessGroup in libs/process/group_other.go only sets WaitDelay. That bounds Wait at timeout + 10s, but grandchildren such as node under codex.cmd are not killed, so the "blocks for up to 20s" bound still won't hold exactly there.

I'll call out the Windows caveat in the Merge Danger section of the description.

@rugpanov rugpanov Nov 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our review found this too. Note: through #7482, runAgentCmd still calls process.Background without process.WithProcessGroup() (libs/aitools/installer/plugin.go:84 in #7482). So the follow-up that you mention has not landed in the stack yet. A tracking issue would keep it from being lost.

@anton-107 anton-107 Nov 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it isn't in the stack yet. As I answered above, it stays a follow-up rather than part of this PR, which only changes the timeout value. Agreed that a tracking issue would keep it from getting lost.

// pluginCmdTimeout bounds an install/update/uninstall command, which may
// clone the marketplace repo, so it gets more headroom than the probe.
pluginCmdTimeout = 60 * time.Second
Expand Down
Loading