From aadab95bde591dfd653e122b843a45018df70041 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Mon, 13 Jul 2026 14:53:01 +0200 Subject: [PATCH] lazy toolbox: serialize reloads with removals; clean the live box on remove CI (integration shard 1) hit the uninstall-resurrect again: after uninstall_repository, GET /api/tools/{guid} still returned the tool. The log shows the interleaving - the reload_toolbox queued by the preceding install's conf write executed while the uninstall was mid-flight, and its rebuild (including a potentially long inline index populate) ran with no synchronization against remove_tool_by_id. Two halves: 1. _get_new_toolbox now builds and swaps the new toolbox under app._toolbox_lock, the lock remove_tool_by_id and invalidate_index_cache already hold. Whichever side wins, the other sees a consistent world: removal-first means the rebuild reads the already-cleaned index and conf; rebuild-first means the removal runs against the swapped-in box. The lock is an RLock, so the populate broadcast delivered inline on the same thread re-enters safely. 2. remove_tool_by_id also cleans the CURRENT app.toolbox when it is a different object than self - a rebuild that won the race swapped in a box built from the pre-removal index, and cleaning the superseded instance alone would leave the live box serving the uninstalled tool. Verified: new unit test (swapped-in box cleaned), 45 lazy unit tests, fastp shed-install + repository-uninstall integration tests in lazy mode (the lock's deadlock canary - install triggers watcher reload + inline populate under the lock), tox lint/format/mypy. --- lib/galaxy/queue_worker/__init__.py | 55 +++++++++++++++------------ lib/galaxy/tools/lazy_toolbox.py | 10 +++++ test/unit/app/tools/test_lazy_tool.py | 19 +++++++++ 3 files changed, 60 insertions(+), 24 deletions(-) diff --git a/lib/galaxy/queue_worker/__init__.py b/lib/galaxy/queue_worker/__init__.py index e71ca5f7c03..a998c012f00 100644 --- a/lib/galaxy/queue_worker/__init__.py +++ b/lib/galaxy/queue_worker/__init__.py @@ -286,33 +286,40 @@ def _get_new_toolbox(app: "UniverseApplication", save_integrated_tool_panel: boo """ Generate a new toolbox, by constructing a toolbox from the config files, and then adding pre-existing data managers from the old toolbox to the new toolbox. + + Serialized under ``app._toolbox_lock`` against ``remove_tool_by_id`` and + ``invalidate_index_cache``: a reload queued by an earlier conf write (the + install that precedes an uninstall) otherwise interleaves its rebuild — + including a potentially long inline index populate — with the removal, + and the swapped-in toolbox resurrects the just-removed tool. """ tool_configs = app.config.tool_configs - new_toolbox: ToolBox - if getattr(app.config, "use_lazy_toolbox", False) and getattr(app, "tool_source_store", None) is not None: - new_toolbox = LazyToolBox( - config_filenames=tool_configs, - tool_root_dir=app.config.tool_path, - app=app, - tool_source_store=app.tool_source_store, - cache_size=getattr(app.config, "lazy_toolbox_cache_size", 500), - save_integrated_tool_panel=save_integrated_tool_panel, - ) - else: - new_toolbox = ToolBox( - tool_configs, - app.config.tool_path, - app, - save_integrated_tool_panel=save_integrated_tool_panel, - ) - new_toolbox.data_manager_tools = app.toolbox.data_manager_tools - app.datatypes_registry.load_datatype_converters(new_toolbox, use_cached=True) - app.datatypes_registry.load_external_metadata_tool(new_toolbox) - load_lib_tools(new_toolbox) - for tool in new_toolbox.data_manager_tools.values(): - new_toolbox.register_tool(tool) - app._toolbox = new_toolbox + with app._toolbox_lock: + new_toolbox: ToolBox + if getattr(app.config, "use_lazy_toolbox", False) and getattr(app, "tool_source_store", None) is not None: + new_toolbox = LazyToolBox( + config_filenames=tool_configs, + tool_root_dir=app.config.tool_path, + app=app, + tool_source_store=app.tool_source_store, + cache_size=getattr(app.config, "lazy_toolbox_cache_size", 500), + save_integrated_tool_panel=save_integrated_tool_panel, + ) + else: + new_toolbox = ToolBox( + tool_configs, + app.config.tool_path, + app, + save_integrated_tool_panel=save_integrated_tool_panel, + ) + new_toolbox.data_manager_tools = app.toolbox.data_manager_tools + app.datatypes_registry.load_datatype_converters(new_toolbox, use_cached=True) + app.datatypes_registry.load_external_metadata_tool(new_toolbox) + load_lib_tools(new_toolbox) + for tool in new_toolbox.data_manager_tools.values(): + new_toolbox.register_tool(tool) + app._toolbox = new_toolbox def reload_data_managers(app, **kwargs): diff --git a/lib/galaxy/tools/lazy_toolbox.py b/lib/galaxy/tools/lazy_toolbox.py index 0d47da823f0..437669c37d1 100644 --- a/lib/galaxy/tools/lazy_toolbox.py +++ b/lib/galaxy/tools/lazy_toolbox.py @@ -1774,6 +1774,16 @@ class LazyToolBox(ToolBox): send_control_task(self.app, "reload_tool_source_cache", noop_self=True) except Exception as e: log.warning("Broadcasting index removal of %s raised: %s", tool_id, e) + # A toolbox reload queued before this removal (the conf write of + # the preceding install) may already have swapped a NEW toolbox + # into ``app.toolbox``, built from the index as it stood before + # ``remove_index_entry`` above — with the tool still registered. + # ``self`` is then the superseded instance and cleaning it alone + # leaves ``app.toolbox`` serving the uninstalled tool. The swap + # holds the same lock, so the current object is stable here. + current = getattr(self.app, "toolbox", None) + if current is not self and isinstance(current, LazyToolBox): + current._remove_tool_in_memory(tool_id, remove_from_panel=remove_from_panel) return result def _remove_tool_in_memory(self, tool_id: str, remove_from_panel: bool = True): diff --git a/test/unit/app/tools/test_lazy_tool.py b/test/unit/app/tools/test_lazy_tool.py index e0fbce9cb9d..e6114d7fba2 100644 --- a/test/unit/app/tools/test_lazy_tool.py +++ b/test/unit/app/tools/test_lazy_tool.py @@ -564,6 +564,25 @@ def test_remove_tool_by_id_broadcasts_reload_to_peers(monkeypatch): assert "doomed" not in box._tools_by_id +def test_remove_tool_by_id_also_cleans_swapped_in_toolbox(monkeypatch): + # A reload queued before the removal can swap a new toolbox into + # app.toolbox, rebuilt from the pre-removal index — cleaning only + # ``self`` would leave the new box serving the uninstalled tool. + old_box = _registry_box() + new_box = _registry_box() + entry = _entry(id="doomed") + for box in (old_box, new_box): + box._tool_index = ToolIndex() + box._tool_index.add_entry(entry) + box._register_lazy_entry(entry) + old_box.app.toolbox = new_box + monkeypatch.setattr(queue_worker_mod, "send_control_task", lambda app, task, **kwargs: None) + old_box.remove_tool_by_id("doomed") + assert "doomed" not in old_box._tools_by_id + assert "doomed" not in new_box._tools_by_id + assert "tool_doomed" not in new_box._tool_panel + + def test_tool_file_on_disk_answers_from_index(tmp_path): box = _seam_box() box._index_source_paths_cache = None