mirror of
https://github.com/galaxyproject/galaxy.git
synced 2026-09-21 05:45:37 +08:00
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.
This commit is contained in:
@@ -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):
|
||||
|
||||
@@ -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):
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user