ag-ui-protocol / ag-ui-protocol/ag-ui

[adk-middleware] Per-request Runner.close() tears down caller-owned App plugins (should set skip_closing_plugins)

未關閉
#2,642 0 則留言 0 個 reaction 已指派 0 人 在 GitHub 檢視
主要語言
Python
星號
15.9k
分支
1.4k
平均合併
1 天 17 小時
30 天內合併 PR
163

描述

**TL;DR** — `ADKAgent` builds a `Runner` per request from a **shallow** copy of the caller's `App`, then closes that `Runner` in a `finally` block. `Runner.close()` closes the plugin list, which the shallow copy shares with the caller. So every request tears down plugin instances the host owns and expects to live for the process. ADK provides `PluginManager.set_skip_closing_plugins(True)` for exactly this case and uses it internally in `agent_tool.py`; the ADK middleware does not. One line fixes it.

---

## For humans

### What happens

`_create_runner` builds a per-request `App` copy and a fresh `Runner` from it:

```python
request_app = self._app.model_copy(update={'root_agent': adk_agent})
return Runner(app=request_app, **service_kwargs)
```

`model_copy` is shallow, so `request_app.plugins` is the same list holding the same plugin objects the caller constructed and passed to `ADKAgent.from_app(...)`.

At the end of the background execution, the runner is closed:

```python
finally:
# Ensure the ADK runner releases any resources (e.g. toolsets)
if runner is not None:
close_method = getattr(runner, "close", None)
...
```

The comment is about toolsets, and closing the runner is the right instinct for those. But `Runner.close()` also calls `PluginManager.close()`, which calls `close()` on every registered plugin. Those plugins are not the runner's to close. They belong to the host's long-lived `App`.

### Why it matters

Plugins are documented as long-lived objects that hold state across invocations: clients, connection pools, background batch writers. Closing them per request means every request pays a full teardown and the next one pays a full re-initialisation.

We hit this in production with `BigQueryAgentAnalyticsPlugin`. Measured over ten minutes on one service:

| Signal | Count |
|---|---|
| Runner closed | 325 |
| Plugin registered into a new manager | 329 |
| Plugin's batch writer drained | 101 |
| Host constructing the plugin | 0 |

That last row is the point. The plugin is constructed once, at import. The middleware cycles it 325 times in ten minutes.

For that particular plugin the consequence was severe, because its re-initialisation path issues 25 `CREATE OR REPLACE VIEW` statements and is awaited on the request path. We exhausted a BigQuery daily quota in two hours and added about 25 seconds to median latency. We have filed that amplification separately against `google/adk-python`, and it should be fixed there too. But the trigger is here, and this side is the cheaper and more general fix: no plugin should be closed by a component that did not create it.

### The guard already exists

ADK anticipated this. `PluginManager.set_skip_closing_plugins(True)` makes `close()` a no-op for plugins owned elsewhere, and `google/adk/tools/agent_tool.py` calls it when it creates a nested `Runner` over shared plugins:

```python
runner.plugin_manager.set_skip_closing_plugins(True)
```

The ADK middleware creates a `Runner` over shared plugins in exactly the same way and should do the same. Toolset cleanup is unaffected, since that is a separate path inside `Runner.close()`.

### Proposed change

In `_create_runner`, on the `self._app is not None` branch only, set the flag on the constructed runner, feature-detected the same way `plugin_close_timeout` already is. When the middleware builds the agent component-wise there is no caller-owned plugin list, so that branch is unchanged.

Happy to send the PR; draft linked below.

---

## For AI agents

```yaml
defect:
id: adk-middleware-closes-caller-owned-plugins
component: ag_ui_adk.ADKAgent
package: ag-ui-adk
version: "0.7.0"
path: integrations/adk-middleware/python
status_at_head: present
head_verified: 8e3e1ad8
class: [resource-lifecycle, ownership-violation, performance]

root_cause: >-
A per-request Runner is constructed from a shallow App.model_copy, so it
shares the caller's plugin instances, and is then closed in a finally block.
Runner.close() -> PluginManager.close() -> plugin.close() therefore tears
down host-owned, process-lifetime plugins once per request.

call_chain:
- file: src/ag_ui_adk/adk_agent.py
symbol: _create_runner
line: 1114
- file: src/ag_ui_adk/adk_agent.py
line: 1143
code: "request_app = self._app.model_copy(update={'root_agent': adk_agent})"
note: "shallow copy; request_app.plugins aliases the caller's plugin objects"
- file: src/ag_ui_adk/adk_agent.py
line: 1144
code: "return Runner(app=request_app, **service_kwargs)"
- file: src/ag_ui_adk/adk_agent.py
line: 3475
code: 'close_method = getattr(runner, "close", None)'
note: "DEFECT SITE — per-request close of a runner holding caller-owned plugins"
- repo: google/adk-python
file: src/google/adk/runners.py
line: 2149
code: "await self.plugin_manager.close()"
- repo: google/adk-python
file: src/google/adk/plugins/plugin_manager.py
line: 404
code: "await plugin.close()"

existing_guard:
api: PluginManager.set_skip_closing_plugins
defined: google/adk-python src/google/adk/plugins/plugin_manager.py:95
honoured: google/adk-python src/google/adk/plugins/plugin_manager.py:392
upstream_precedent: google/adk-python src/google/adk/tools/agent_tool.py:278
called_in_this_repo: false

evidence:
window: "10 minutes, one production service, ADK 2.8.0"
runner_close_events: 325
plugin_register_events: 329
plugin_batch_writer_drains: 101
host_plugin_constructor_invocations: 0
fork_events: 0
instance_starts: 0
interpretation: >-
A single shared plugin instance was closed and re-initialised once per
request; the host never rebuilt it.

proposed_fix:
site: "src/ag_ui_adk/adk_agent.py:_create_runner, self._app is not None branch"
change: >-
After constructing the Runner, mark its plugin manager as not owning the
plugins, feature-detected for ADK versions predating the API, mirroring
the existing _runner_supports_plugin_close_timeout pattern.
sketch: |
runner = Runner(app=request_app, **service_kwargs)
plugin_manager = getattr(runner, "plugin_manager", None)
setter = getattr(plugin_manager, "set_skip_closing_plugins", None)
if setter is not None:
setter(True)
return runner
unchanged: "component-based branch (no caller-owned plugin list)"
unaffected: "toolset cleanup, which is a separate path inside Runner.close()"

acceptance_criteria:
- "a plugin passed via App is not closed when a per-request Runner is closed"
- "toolsets are still released on runner close"
- "no behaviour change when the installed ADK predates set_skip_closing_plugins"
- "component-based construction path is untouched"

related:
- "google/adk-python: BigQueryAgentAnalyticsPlugin re-runs view DDL on every re-initialisation (amplifies this defect)"
```

貢獻指南

開啟貢獻指南

評估

這個 Issue 還沒有評估資料。

把新 issue 寄到你的電子郵件信箱

精選適合新手參與的 GitHub issue 摘要。