From 133232dc0c6d37b6fc82e24fa48a3ef378627186 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 2 May 2026 05:35:05 +0000 Subject: [PATCH 1/2] Move dock client status reads off main; cache uvx --version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dock's `_perform_initial_client_status_refresh` did ~16 sync `FileAccess.open` + `JSON.parse_string` calls on the editor main thread before the loading bar finished, justified as a pre-warm of GDScript's lazy bytecode swap (#233 / #235). The warm only needs to dereference each strategy script once — the 16 disk reads were incidental. Replace with an explicit `_warm_strategy_bytecode` helper that invokes a pure-memory entry point on each of `_json_strategy.gd`, `_toml_strategy.gd`, `_cli_strategy.gd` plus `client_configurator.gd`. No disk, no `OS.execute`. Then route every per-client probe (JSON, TOML, CLI) through the existing worker thread so the dock paints immediately on cold open. Also cache `uvx --version` for the editor session (mirror of the existing `_cached_venv_python` pattern). The dock's `_refresh_setup_status` reads it on the main thread on every dock build and focus-in refresh; ~80 ms cold on Linux, more on Windows. Cache invalidator wired into `_on_install_uv` alongside `McpCliFinder.invalidate("uvx")` so a fresh install is picked up. Trace verification (Linux 4.6.2, free port): Before: dock_attached=397 ms, total=436 ms After: dock_attached=230 ms, total=279 ms (−167 ms, −36 %) Tests: - Updated `test_initial_paint_warms_worker_call_graph_before_threading` to assert the new shape: `_warm_strategy_bytecode()` is called, every config_type is batched into the worker, no sync per-client status probe runs on main, the warm helper stays pure-memory. - Added `test_check_uv_version_caches_for_session` asserting the cache + invalidator + dock install-flow wiring. --- plugin/addons/godot_ai/client_configurator.gd | 32 +++- plugin/addons/godot_ai/mcp_dock.gd | 62 ++++-- tests/unit/test_editor_focus_refocus.py | 177 ++++++++++++++---- 3 files changed, 217 insertions(+), 54 deletions(-) diff --git a/plugin/addons/godot_ai/client_configurator.gd b/plugin/addons/godot_ai/client_configurator.gd index d29a948f..eb1cf6fa 100644 --- a/plugin/addons/godot_ai/client_configurator.gd +++ b/plugin/addons/godot_ai/client_configurator.gd @@ -465,14 +465,42 @@ static func find_uvx() -> String: return McpCliFinder.find(names) +static var _uv_version_cache: String = "" +static var _uv_version_searched: bool = false + + +## Cached for the editor session. The dock's `_refresh_setup_status` +## (called via `call_deferred` from `_build_ui`) calls this on the +## main thread in user mode, so a single cold `OS.execute(uvx, +## ["--version"])` adds ~80 ms to the dock's first paint on Linux and +## more on Windows. Subsequent calls (focus-in refresh, manual Refresh +## clicks) reuse the cached string. +## +## Invalidate via `invalidate_uv_version_cache()` when the user +## installs / reinstalls uv via the dock so the next refresh reflects +## the new install. The dock's `_on_install_uv` calls this alongside +## `McpCliFinder.invalidate("uvx")` to clear both the path cache and +## the version cache in one place. static func check_uv_version() -> String: + if _uv_version_searched: + return _uv_version_cache var uvx := find_uvx() if uvx.is_empty(): + _uv_version_searched = true + _uv_version_cache = "" return "" var output: Array = [] if OS.execute(uvx, ["--version"], output, true) == 0 and output.size() > 0: - return output[0].strip_edges() - return "" + _uv_version_cache = output[0].strip_edges() + else: + _uv_version_cache = "" + _uv_version_searched = true + return _uv_version_cache + + +static func invalidate_uv_version_cache() -> void: + _uv_version_searched = false + _uv_version_cache = "" static var _venv_python_cache: String = "" diff --git a/plugin/addons/godot_ai/mcp_dock.gd b/plugin/addons/godot_ai/mcp_dock.gd index 41403b02..db73d9d6 100644 --- a/plugin/addons/godot_ai/mcp_dock.gd +++ b/plugin/addons/godot_ai/mcp_dock.gd @@ -1318,6 +1318,11 @@ func _on_install_uv() -> void: OS.execute("powershell", ["-ExecutionPolicy", "ByPass", "-c", "irm https://astral.sh/uv/install.ps1 | iex"], [], false) _: OS.execute("bash", ["-c", "curl -LsSf https://astral.sh/uv/install.sh | sh"], [], false) + ## Drop the cached uvx path AND the cached `uvx --version` so the + ## next `_refresh_setup_status` finds and reads the freshly-installed + ## binary instead of returning the pre-install "not found" result. + McpCliFinder.invalidate("uvx") + McpClientConfigurator.invalidate_uv_version_cache() _refresh_setup_status.call_deferred() @@ -1852,7 +1857,8 @@ func _prune_orphaned_client_status_refresh_threads() -> void: func _perform_initial_client_status_refresh() -> void: - ## Pre-warm strategy bytecode on main, defer CLI probes to the worker. + ## Pre-warm strategy bytecode on main, then hand every client probe + ## (JSON / TOML / CLI alike) to the worker. ## ## Godot's GDScript hot-reload of overwritten plugin files is lazy: the ## bytecode swap happens on first dereference, not at `set_plugin_enabled` @@ -1865,10 +1871,19 @@ func _perform_initial_client_status_refresh() -> void: ## in-place plugin reload because the editor stays focused — so neither ## works as a gate. See #233 / #235. ## - ## Phase 1 (sync, on main): for each client, snapshot warms the CLI call - ## graph via `resolve_cli_path`; for non-CLI clients, sync `check_status` - ## warms `_json_strategy.gd` / `_toml_strategy.gd`. Phase 2 (worker): CLI - ## probes only, race-safe because Phase 1 dereferenced their call graph. + ## Phase 1 (sync, on main): a single explicit `_warm_strategy_bytecode` + ## call invokes a pure-memory helper on each strategy script — + ## `_json_strategy.gd`, `_toml_strategy.gd`, `_cli_strategy.gd`, plus + ## `client_configurator.gd` via `client_ids()` / `get_by_id`. No disk, + ## no `OS.execute`, no JSON parse on main. `client_status_probe_snapshot` + ## per client adds the `installed` flag and (for CLI clients) a cached + ## CLI path to each probe. + ## + ## Phase 2 (worker): every probe — JSON, TOML, CLI — runs through the + ## same `_run_client_status_refresh_worker` pipeline. Disk reads + JSON + ## parses for the ~17 non-CLI clients now happen off the main thread, + ## so the dock paints immediately on cold open instead of stalling + ## behind ~16 sync `FileAccess.open` + `JSON.parse_string` calls. ## ## No-op outside the tree — GDScript tests instantiate via `new()`. if not is_inside_tree(): @@ -1891,36 +1906,27 @@ func _perform_initial_client_status_refresh() -> void: _refresh_clients_summary() return + _warm_strategy_bytecode() + var generation := _begin_client_status_refresh_run() var server_url := McpClientConfigurator.http_url() - var deferred_cli_probes: Array[Dictionary] = [] + var all_probes: Array[Dictionary] = [] for client_id in _client_rows: - var client := McpClientRegistry.get_by_id(String(client_id)) - if client == null: - continue var probe := McpClientConfigurator.client_status_probe_snapshot(String(client_id)) if probe.is_empty(): continue - if client.config_type == "cli": - deferred_cli_probes.append(probe) - continue - var status := McpClientConfigurator.check_status_for_url_with_cli_path( - String(client_id), server_url, "" - ) - _apply_row_status( - String(client_id), status, "", bool(probe.get("installed", false)) - ) + all_probes.append(probe) _refresh_clients_summary() - if deferred_cli_probes.is_empty(): + if all_probes.is_empty(): _finalize_completed_refresh() return _client_status_refresh_thread = Thread.new() var err := _client_status_refresh_thread.start( Callable(self, "_run_client_status_refresh_worker").bind( - deferred_cli_probes, server_url, generation + all_probes, server_url, generation ) ) if err != OK: @@ -1930,6 +1936,22 @@ func _perform_initial_client_status_refresh() -> void: _refresh_clients_summary() +## Force GDScript's lazy bytecode swap to complete for every script the +## worker thread will reach into. Each call is pure-memory — no disk, no +## network, no `OS.execute` — so it only costs the bytecode dereference +## itself. See `_perform_initial_client_status_refresh` for context and +## #233 / #235 for the SIGABRT this exists to prevent. +func _warm_strategy_bytecode() -> void: + var ids := McpClientConfigurator.client_ids() + if ids.is_empty(): + return + var any_client := McpClientRegistry.get_by_id(String(ids[0])) + if any_client != null: + McpJsonStrategy.verify_entry(any_client, {}, "") + McpTomlStrategy.format_body(PackedStringArray(), "") + McpCliStrategy.format_args(PackedStringArray(), "", "") + + func _begin_client_status_refresh_run() -> int: ## Marks a refresh as starting and returns the new generation token. ## Generation is bumped here (not at completion) so that a worker callback diff --git a/tests/unit/test_editor_focus_refocus.py b/tests/unit/test_editor_focus_refocus.py index 28b83e31..a00fa6b0 100644 --- a/tests/unit/test_editor_focus_refocus.py +++ b/tests/unit/test_editor_focus_refocus.py @@ -52,11 +52,12 @@ def test_clients_window_open_requests_nonblocking_refresh() -> None: def test_initial_paint_warms_worker_call_graph_before_threading() -> None: - """Cold editor open pre-warms strategy bytecode on main, then defers CLI to worker (#235). + """Cold editor open pre-warms strategy bytecode on main, then hands every probe to the worker. - Deterministic replacement for the prior 1.5s settle timer (#234), with the - cold-start hitch minimized so #228's responsiveness win for focus-in / - refocus paths is not regressed. + Deterministic replacement for the prior 1.5s settle timer (#234), with + every per-client status probe (JSON / TOML / CLI alike) routed through + the existing worker so cold-start dock paint isn't blocked behind ~16 + sync `FileAccess.open` + `JSON.parse_string` calls. The race: Godot's lazy GDScript hot-reload of overwritten plugin files swaps bytecode on first dereference. A worker spawned from a fresh @@ -67,19 +68,22 @@ def test_initial_paint_warms_worker_call_graph_before_threading() -> None: thread *before* the worker starts. After this helper, bytecode is stable everywhere the worker reaches → no race possible. - • `client_status_probe_snapshot` (called per client on main) warms - `_cli_strategy.gd` / `_cli_finder.gd` for CLI clients via - `resolve_cli_path`. - • Sync `check_status_for_url_with_cli_path` on JSON/TOML clients - (file-read + parse, ~5–20ms each) warms `_json_strategy.gd` / - `_toml_strategy.gd` and the configurator dispatch. - • CLI clients are bundled into a deferred batch and probed by the - worker — slow `OS.execute` calls stay off-thread, preserving #228. - - The structural assertions below lock in this hybrid: a future "make - startup snappier" refactor can't drop the warming step (re-introducing - the race), and a future "be more conservative" refactor can't move the - CLI probes back on-thread (regressing #228's responsiveness fix). + • A single explicit `_warm_strategy_bytecode()` call invokes a + pure-memory helper (no disk, no `OS.execute`) on each strategy + script — `_json_strategy.gd`, `_toml_strategy.gd`, `_cli_strategy.gd` + — plus `client_configurator.gd` via `client_ids()` / `get_by_id`. + • `client_status_probe_snapshot` (called per client on main) builds + the per-row probe envelope with the `installed` flag and (for CLI + clients) the cached CLI path. + • Every probe — JSON, TOML and CLI — is then handed to the same + worker thread that already handles CLI probes. Disk reads + JSON + parses for the ~17 non-CLI clients now happen off the main thread. + + The structural assertions below lock in this contract: a future "make + startup snappier" refactor can't drop the explicit warming step + (re-introducing #233's race), and a future "be more conservative" + refactor can't move the per-client status reads back on-thread + (regressing #228's responsiveness fix and re-blocking the cold paint). """ source = (PLUGIN_ROOT / "mcp_dock.gd").read_text() @@ -91,22 +95,32 @@ def test_initial_paint_warms_worker_call_graph_before_threading() -> None: helper_block = source.split("func _perform_initial_client_status_refresh() -> void:", 1)[ 1 ].split("\n\nfunc ", 1)[0] + assert "_warm_strategy_bytecode()" in helper_block, ( + "Helper must call _warm_strategy_bytecode() before spawning the " + "worker — that's the single explicit dereference of every strategy " + "script the worker will reach into. Removing it re-introduces #233's " + "lazy hot-reload SIGABRT race." + ) assert "client_status_probe_snapshot(" in helper_block, ( - "Helper must call client_status_probe_snapshot per client on main — " - "this is what dereferences `_cli_strategy.gd` / `_cli_finder.gd` for " - "CLI clients before the worker spawns. See #235." - ) - assert "check_status_for_url_with_cli_path(" in helper_block, ( - "Helper must run a sync check_status_for_url_with_cli_path for " - "JSON/TOML clients on main — that warms `_json_strategy.gd` / " - "`_toml_strategy.gd` so the worker (if it spawns) can't race the " - "lazy hot-reload bytecode swap. See #235." - ) - assert "deferred_cli_probes" in helper_block, ( - "Helper must batch CLI probes for the deferred (worker) phase. " - "Without this split, the cold-start path either (a) runs CLI probes " - "on main and re-introduces #228's per-focus editor freezes, or " - "(b) skips warming and races GDScript hot-reload (#233)." + "Helper must call client_status_probe_snapshot per client on main " + "to build each row's probe envelope (installed flag + cached CLI " + "path). See #235." + ) + assert "check_status_for_url_with_cli_path(" not in helper_block, ( + "Helper must not run sync per-client status probes on main — that " + "blocks the dock's cold paint behind ~16 FileAccess + JSON.parse " + "calls. The worker handles JSON/TOML/CLI alike via " + "_run_client_status_refresh_worker." + ) + assert "all_probes" in helper_block, ( + "Helper must batch every probe (JSON + TOML + CLI) into one list " + "for the worker thread. Splitting them would partially regress the " + "cold-paint win or re-introduce the SIGABRT race for the moved-" + "back-on-main clients." + ) + assert "deferred_cli_probes" not in helper_block, ( + "The CLI-only batch is gone — the worker now handles every config " + "type, so a CLI-specific staging list is dead code." ) assert "await " not in helper_block, ( "Helper must be a single straight-line block — no timer awaits, no " @@ -118,6 +132,25 @@ def test_initial_paint_warms_worker_call_graph_before_threading() -> None: "from #234 that #235 replaces)." ) + warm_block = source.split("func _warm_strategy_bytecode() -> void:", 1)[1].split( + "\n\nfunc ", 1 + )[0] + assert "McpJsonStrategy." in warm_block, ( + "_warm_strategy_bytecode must dereference McpJsonStrategy so the " + "worker can't race the JSON strategy's lazy bytecode swap." + ) + assert "McpTomlStrategy." in warm_block, ( + "_warm_strategy_bytecode must dereference McpTomlStrategy." + ) + assert "McpCliStrategy." in warm_block, ( + "_warm_strategy_bytecode must dereference McpCliStrategy." + ) + assert "FileAccess" not in warm_block and "OS.execute" not in warm_block, ( + "_warm_strategy_bytecode must stay pure-memory — no disk, no " + "subprocess. The point is to dereference scripts cheaply, not to " + "re-introduce the per-client disk reads we just moved to the worker." + ) + constants_block = source.split("class_name McpDock", 1)[1].split("\nvar ", 1)[0] assert "CLIENT_STATUS_REFRESH_INITIAL_DELAY_MSEC" not in constants_block, ( "The settle-timer constant from #234 must be removed — keeping it " @@ -496,6 +529,86 @@ def test_refresh_timeout_can_abandon_stale_worker_results() -> None: assert "generation != _client_status_refresh_generation" in source +def test_check_uv_version_caches_for_session() -> None: + """`uvx --version` must run at most once per editor session. + + The dock's `_refresh_setup_status` calls `McpClientConfigurator.check_uv_version()` + on the main thread (via `call_deferred` from `_build_ui`) in user mode. + Each cold call costs an `OS.execute("uvx", ["--version"])` round-trip + (~80 ms on Linux, more on Windows) — multiplied by every focus-in + refresh and every manual Refresh click that's a real cost on the + dock's first paint and on every responsiveness gate after. + + Cache it the same way `_cached_venv_python` already works + (`_venv_python_cache` + `_venv_python_searched` pair). Invalidate + only when the user installs / reinstalls uv via the dock — the + `McpCliFinder.invalidate("uvx")` site is the natural sibling, so + a single explicit `invalidate_uv_version_cache()` call clears both. + """ + + source = (PLUGIN_ROOT / "client_configurator.gd").read_text() + + assert "static var _uv_version_cache: String" in source, ( + "Cached `uvx --version` string must be a class-level static so it " + "survives across plugin reloads and dock rebuilds." + ) + assert "static var _uv_version_searched: bool" in source, ( + "Companion 'have we searched yet?' flag must be a class-level " + "static — empty cache is ambiguous (means both 'never asked' and " + "'asked, uv not installed') without it." + ) + + helper_block = source.split("static func check_uv_version() -> String:", 1)[1].split( + "\n\n", 1 + )[0] + assert "if _uv_version_searched:" in helper_block, ( + "First line of check_uv_version must short-circuit on the cached " + "result. Otherwise the cache is doing nothing." + ) + assert "return _uv_version_cache" in helper_block, ( + "The short-circuit must return the cached string, not re-derive it." + ) + assert "_uv_version_searched = true" in helper_block, ( + "Every code path that calls OS.execute or short-circuits 'uv not " + "found' must set _uv_version_searched = true. Otherwise the next " + "call re-runs OS.execute, defeating the cache." + ) + + assert "static func invalidate_uv_version_cache() -> void:" in source, ( + "An explicit invalidator must exist so the dock's _on_install_uv " + "can drop the cached 'uv not found' result after a successful " + "install." + ) + + invalidator_block = source.split( + "static func invalidate_uv_version_cache() -> void:", 1 + )[1].split("\n\n", 1)[0] + assert "_uv_version_searched = false" in invalidator_block, ( + "Invalidator must reset _uv_version_searched, otherwise the next " + "call short-circuits on the stale cached value." + ) + assert "_uv_version_cache = " in invalidator_block, ( + "Invalidator must clear the cached string too — leaving stale data " + "would surface in any path that reads the cache without going " + "through check_uv_version (e.g. future inspectors / debug helpers)." + ) + + dock_source = (PLUGIN_ROOT / "mcp_dock.gd").read_text() + install_block = dock_source.split("func _on_install_uv() -> void:", 1)[1].split( + "\n\nfunc ", 1 + )[0] + assert 'McpCliFinder.invalidate("uvx")' in install_block, ( + "_on_install_uv must invalidate the CLI path cache after running " + "the installer, otherwise find_uvx() returns the pre-install " + "'not found' value forever." + ) + assert "McpClientConfigurator.invalidate_uv_version_cache()" in install_block, ( + "_on_install_uv must invalidate the version cache too — without " + "this, the dock's setup status keeps showing 'uv: not found' " + "after a successful install." + ) + + def test_configure_all_uses_cached_status_not_dot_color() -> None: """Configure-all must not make correctness decisions from stale UI colors.""" From a0d1042b670efedcf7c32d8b6e0df80accf5ba8a Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 2 May 2026 05:49:28 +0000 Subject: [PATCH 2/2] Address Copilot review: invalidate Windows uvx.exe cache key The original `_on_install_uv` hardcoded `McpCliFinder.invalidate("uvx")`, but `find_uvx()` caches under `"uvx.exe"` on Windows. After a fresh uv install on Windows, the CLI-path cache stayed stale and the dock would keep showing "uv: not found" for the rest of the session. Add a `McpClientConfigurator.invalidate_uvx_cli_cache()` helper that routes through the same `_uvx_cli_names()` array `find_uvx()` uses, so the OS-specific binary name (uvx vs uvx.exe) stays in lockstep between the populator and the invalidator. `_on_install_uv` now calls the configurator helper instead of the finder directly. Tests updated to assert the routing. --- plugin/addons/godot_ai/client_configurator.gd | 18 +++++++++++++++++- plugin/addons/godot_ai/mcp_dock.gd | 6 +++++- tests/unit/test_editor_focus_refocus.py | 19 +++++++++++++++---- 3 files changed, 37 insertions(+), 6 deletions(-) diff --git a/plugin/addons/godot_ai/client_configurator.gd b/plugin/addons/godot_ai/client_configurator.gd index eb1cf6fa..08a7a5cd 100644 --- a/plugin/addons/godot_ai/client_configurator.gd +++ b/plugin/addons/godot_ai/client_configurator.gd @@ -460,9 +460,25 @@ static func get_server_launch_mode() -> String: static func find_uvx() -> String: + return McpCliFinder.find(_uvx_cli_names()) + + +static func _uvx_cli_names() -> Array[String]: var names: Array[String] = [] names.append("uvx.exe" if OS.get_name() == "Windows" else "uvx") - return McpCliFinder.find(names) + return names + + +## Drop the `McpCliFinder` cache for the platform-specific uvx binary +## name. Pairs with `invalidate_uv_version_cache()` so the dock's +## `_on_install_uv` can refresh both caches with one call each. The +## OS-specific name matters: Windows caches under `uvx.exe`, every +## other platform under `uvx`; hard-coding `"uvx"` here would leave +## the CLI-path cache stale on Windows after a fresh install and the +## dock would keep showing "uv: not found" for the rest of the session. +static func invalidate_uvx_cli_cache() -> void: + for name in _uvx_cli_names(): + McpCliFinder.invalidate(name) static var _uv_version_cache: String = "" diff --git a/plugin/addons/godot_ai/mcp_dock.gd b/plugin/addons/godot_ai/mcp_dock.gd index db73d9d6..8480d7ec 100644 --- a/plugin/addons/godot_ai/mcp_dock.gd +++ b/plugin/addons/godot_ai/mcp_dock.gd @@ -1321,7 +1321,11 @@ func _on_install_uv() -> void: ## Drop the cached uvx path AND the cached `uvx --version` so the ## next `_refresh_setup_status` finds and reads the freshly-installed ## binary instead of returning the pre-install "not found" result. - McpCliFinder.invalidate("uvx") + ## Routing through the configurator here matters on Windows, where + ## the CLI-finder cache key is `uvx.exe` — invalidating just `"uvx"` + ## would leave the cache stale and the dock would keep showing + ## "uv: not found" for the rest of the session. + McpClientConfigurator.invalidate_uvx_cli_cache() McpClientConfigurator.invalidate_uv_version_cache() _refresh_setup_status.call_deferred() diff --git a/tests/unit/test_editor_focus_refocus.py b/tests/unit/test_editor_focus_refocus.py index a00fa6b0..095284bc 100644 --- a/tests/unit/test_editor_focus_refocus.py +++ b/tests/unit/test_editor_focus_refocus.py @@ -597,10 +597,11 @@ def test_check_uv_version_caches_for_session() -> None: install_block = dock_source.split("func _on_install_uv() -> void:", 1)[1].split( "\n\nfunc ", 1 )[0] - assert 'McpCliFinder.invalidate("uvx")' in install_block, ( - "_on_install_uv must invalidate the CLI path cache after running " - "the installer, otherwise find_uvx() returns the pre-install " - "'not found' value forever." + assert "McpClientConfigurator.invalidate_uvx_cli_cache()" in install_block, ( + "_on_install_uv must invalidate the CLI-path cache via the " + "configurator helper (which knows the OS-specific binary name). " + "A direct `McpCliFinder.invalidate(\"uvx\")` would leave the " + "Windows cache stale — Windows caches under `uvx.exe`." ) assert "McpClientConfigurator.invalidate_uv_version_cache()" in install_block, ( "_on_install_uv must invalidate the version cache too — without " @@ -608,6 +609,16 @@ def test_check_uv_version_caches_for_session() -> None: "after a successful install." ) + cli_invalidator_block = source.split( + "static func invalidate_uvx_cli_cache() -> void:", 1 + )[1].split("\n\n", 1)[0] + assert "_uvx_cli_names()" in cli_invalidator_block, ( + "invalidate_uvx_cli_cache must route through the same " + "_uvx_cli_names() helper that find_uvx() uses, so the OS-" + "specific binary name (uvx vs uvx.exe) stays in lockstep " + "between the populator and the invalidator." + ) + def test_configure_all_uses_cached_status_not_dot_color() -> None: """Configure-all must not make correctness decisions from stale UI colors."""