Repository navigation
aitools: raise the plugin probe timeout from 5s to 20s - #7472
Conversation
2e276b6 to
848f252
Compare
cc3a109 to
3e0bf18
Compare
✅ Integration test reportCommit: b221ddf
Top 19 slowest tests (at least 2 minutes):
|
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Automated review from simonfaltum's review agent (pr-review skill) at 3e0bf18. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 1 open design question.
This PR raises the aitools plugin probe timeout from 5s to 20s so that a slow agent CLI cold start isn't reported as unavailable, and adds a changelog fragment. The change is correct and easy to revert, CI is green, and the description lays out the trade-offs. One question is open: whether a timed-out probe should get its own reason, since it is still reported as cli-not-on-path and telemetry can't confirm the fix. Beyond that, there are a few non-blocking cleanups: the stale 5s constant in agents/detect.go, the scope of the constant's comment, the changelog wording, and the PR description template.
Questions
- A probe that times out is still reported as
cli-not-on-path. Everyplugin --helperror maps toReasonCLINotOnPathinplugin.go:228-229, and the killed process returnssignal: killedrather thancontext.DeadlineExceeded, so telemetry records CLI_NOT_ON_PATH instead of the existing TIMEOUT category. Raising the limit moves the cutoff, but nothing will show whether 20s is enough, and a hung agent CLI now costs 20s per plugin agent instead of 5s. Do you want to keep the constant bump as is, add a distinct timeout reason (here or in a follow-up) so the funnel telemetry can confirm the fix, or treat a probe timeout as inconclusive and let the 60s install step decide?
Comments outside the changed lines
- [nit]
.github/PULL_REQUEST_TEMPLATE.md: The description uses## Summary/## Evidence/## Merge Danger..agents/skills/pr-checklist/SKILL.mdasks agent-authored PRs to follow.github/PULL_REQUEST_TEMPLATE.mdexactly (## Changes,## Why,## Tests) and not to add sections such as## Summary. Most recently merged PRs use the template. Could you move Summary into Changes/Why and Evidence into Tests?
| // 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. | ||
| pluginProbeTimeout = 20 * time.Second |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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)
| // 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. | ||
| pluginProbeTimeout = 20 * time.Second |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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)
| @@ -0,0 +1 @@ | |||
| * `databricks aitools install` gives a coding agent's CLI 20 seconds instead of 5 to answer the plugin capability check, so a slow first start no longer reports the agent as unavailable. ([#7472](https://github.com/databricks/cli/pull/7472)) | |||
There was a problem hiding this comment.
[nit] Three small things in this fragment. (1) The same probe runs in databricks aitools update when it migrates raw skills to the plugin (cmd/aitools/update.go:253 → InstallPluginForAgent), but the entry names only install. (2) "a slow first start no longer reports the agent as unavailable" is true only for starts under 20s. (3) .agents/skills/pr-checklist/SKILL.md asks for an imperative verb or "<command> now …". Suggested wording: "* 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)"
There was a problem hiding this comment.
Switched the fragment to your suggested wording. It names install and update, says "less likely", and keeps the PR link. (410f869)
3e0bf18 to
410f869
Compare
848f252 to
90c8726
Compare
410f869 to
88362b5
Compare
90c8726 to
125c8ca
Compare
88362b5 to
679dd3e
Compare
125c8ca to
0148734
Compare
679dd3e to
ca64d39
Compare
0148734 to
7d1dfc8
Compare
ca64d39 to
9d4b058
Compare
|
Replies to the two review-body items: Question 1 (timed-out probe reported as PR description template: I've kept the current format, because every PR in this stack uses the same Summary / Evidence / Merge Danger layout and that keeps them easy to compare. |
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Automated review from simonfaltum's review agent (pr-review skill) at 9d4b058. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 1 open design question.
This PR raises the aitools plugin probe timeout from 5s to 20s, so a slow agent CLI cold start is less likely to be reported as unavailable. It also updates the unused copy in agents/detect.go and adds a changelog fragment. The change is correct, CI is green, and every point from the earlier review has been addressed or answered. Nothing is blocking. One new non-blocking item: on Windows, npm wrapper CLIs aren't actually bounded by this timeout, because runAgentCmd doesn't use process.WithProcessGroup(). One question remains about the leftover ProbePlugin constant.
Questions
(*Agent).ProbePlugininlibs/aitools/agents/detect.gostill has only test callers, so itspluginProbeTimeoutis a second copy kept equal to the installer's value only by a comment. Do you expectProbePluginto get a production caller? If not, would you delete it with its constant in a follow-up, or haveinstallerandagentsshare one exported constant, so there's a single probe timeout to tune? Keeping both copies as they are is also fine if you'd rather.
Agreeing with existing threads
- [nice to have] A timed-out probe is still reported as cli-not-on-path: #7472 (review)
- [nit] The PR description doesn't use the repo's PR template: #7472 (review)
| // 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 |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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 exactprocess.Backgroundcall (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,
WithProcessGroupinlibs/process/group_other.goonly setsWaitDelay. That boundsWaitat timeout + 10s, but grandchildren such as node undercodex.cmdare 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.
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
7d1dfc8 to
b53927b
Compare
9d4b058 to
149db0e
Compare
b53927b to
14f43cd
Compare
149db0e to
3edb030
Compare
14f43cd to
8e754f4
Compare
d03528e to
0aed2cb
Compare
005fc18 to
f251254
Compare
|
On Question 1: I'll keep |
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Automated review from simonfaltum's review agent (pr-review skill) at f251254. Each finding was verified against the code; simonfaltum may follow up.
No blocking issues found, approving.
This PR raises the aitools plugin probe timeout from 5s to 20s, in the installer and in the unused agents.ProbePlugin copy, and adds a changelog fragment. The commit hasn't changed since the last review round. CI is green, and the earlier fixes (both constants, the comment on marketplace list, the changelog wording) are correct. Nothing is blocking. The open items were all discussed before: the hung-grandchild case in runAgentCmd, which also affects npm-installed Codex on Linux and macOS and has no fix in the stack yet; the cli-not-on-path reason for a timeout, which #7475 fixes; and the PR template.
Agreeing with existing threads
- [nice to have] runAgentCmd doesn't bound a hung grandchild, on Linux and macOS too: #7472 (comment)
- [nice to have] A timed-out probe is still reported as cli-not-on-path: #7472 (review)
- [nit] The PR description doesn't follow the repo's PR template: #7472 (review)
0aed2cb to
9dd6d21
Compare
f251254 to
3e979d0
Compare
9dd6d21 to
7e2cf0e
Compare
3e979d0 to
99db1f7
Compare
7e2cf0e to
2920789
Compare
99db1f7 to
07cf845
Compare
2920789 to
66114a4
Compare
07cf845 to
d82eb6b
Compare
66114a4 to
7cd9908
Compare
d82eb6b to
e9ddafa
Compare
7cd9908 to
fdd8c89
Compare
e9ddafa to
b221ddf
Compare
fdd8c89 to
e10129b
Compare
b221ddf to
78b9500
Compare
e10129b to
5576f2b
Compare
78b9500 to
55c0421
Compare
5576f2b to
151571c
Compare
55c0421 to
5cb043a
Compare
151571c to
64f749d
Compare
`databricks aitools install` runs `<agent> plugin --help` to check that the agent's CLI can install plugins. Agent CLIs are large Node/Rust binaries, and a cold start (first run, antivirus scan, slow container disk) can take several seconds. When the probe times out, the agent is reported as unavailable. ```diff // libs/aitools/installer/plugin.go -pluginProbeTimeout = 5 * time.Second +pluginProbeTimeout = 20 * time.Second ``` The same constant also bounds `<agent> plugin marketplace list`. Changelog fragment: `.nextchanges/cli/aitools-plugin-probe-timeout.md`. Co-authored-by: Isaac <no-reply@databricks.com>
5cb043a to
d27386e
Compare
Stacked on #7471.
Stacked on: aitools: mention databricks aitools install in version --check
Summary
databricks aitools installruns<agent> plugin --helpto check that the agent's CLI can install plugins. Agent CLIs are large Node/Rust binaries, and a cold start (first run, antivirus scan, slow container disk) can take several seconds. When the probe times out, the agent is reported as unavailable.The same constant also bounds
<agent> plugin marketplace list. Changelog fragment:.nextchanges/cli/aitools-plugin-probe-timeout.md.Evidence
A temporary test (not committed) points
probePluginCLIat a fakeclaudescript that sleeps 7s and then prints usage. Linux, at this PR's head:go test ./libs/aitools/...passes. No committed test pins the timeout value, because such a test would have to wait out the timeout.Merge Danger
Door: two-way
Reverting is a one-line change.
Blast Radius: small
aitools installfor up to 20s at the probe instead of 5s. The same applies toplugin marketplace list, where a timeout makes install assume the marketplace is already registered..cmdshim, andrunAgentCmdkills only the direct child on timeout. A hung grandchild that keeps stdout/stderr open can make the probe wait past 20s. This predates the PR; bounding it withprocess.WithProcessGroup()is left to a follow-up.cli-not-on-pathreason as before.agents.ProbePlugininlibs/aitools/agents/detect.gohas its own copy of the constant, also raised to 20s. It has no non-test callers.This pull request and its description were written by Isaac.