From 68f579b63801d647fdc3dd81495810049139cb8a Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Wed, 8 Jul 2026 16:08:39 +0200 Subject: [PATCH] lazy boot: answer the shed-conf walk from the index, not the filesystem MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Walking a CVMFS shed conf, the eager _load_tool_tag_set paid two per-tool costs the lazy toolbox doesn't need: an os.path.exists stat (network round trip on CVMFS — minutes over ~10k tools) and, since conf-provided repositories have no install-DB row by design, one 'Attempted to load tool shed tool, but the repository ... was not found in database' warning per tool per boot. Two overridable hooks in AbstractToolBox, both eager-behavior-preserving: _tool_file_on_disk (the existence gate) and _missing_repository_log_level (the warning's severity). LazyToolBox answers both from an identity-cached set of indexed source_paths: the populator established existence when it stored the source and materialisation reads the raw source from the store, so the stat proves nothing; the missing install-DB row is expected, so it logs at debug. Unindexed paths keep exact eager behavior. Claude-Session: https://claude.ai/code/session_018L7ZmCv2ubKA3JNeSL8Pkr --- lib/galaxy/tool_util/toolbox/base.py | 24 +++++++++++- lib/galaxy/tools/lazy_toolbox.py | 53 +++++++++++++++++++++++++++ test/unit/app/tools/test_lazy_tool.py | 32 ++++++++++++++++ 3 files changed, 107 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/tool_util/toolbox/base.py b/lib/galaxy/tool_util/toolbox/base.py index 39f7f8cbfd2..c71d483dc77 100644 --- a/lib/galaxy/tool_util/toolbox/base.py +++ b/lib/galaxy/tool_util/toolbox/base.py @@ -990,7 +990,7 @@ class AbstractToolBox(ManagesIntegratedToolPanelMixin): template_kwds = self._path_template_kwds() path = string.Template(path_template).safe_substitute(**template_kwds) concrete_path = os.path.join(tool_path, path) - if not os.path.exists(concrete_path): + if not self._tool_file_on_disk(concrete_path): # This is a lot faster than attempting to load a non-existing tool raise OSError(ENOENT, os.strerror(ENOENT)) tool_shed_repository = None @@ -1047,6 +1047,26 @@ class AbstractToolBox(ManagesIntegratedToolPanelMixin): except Exception: log.exception("Error reading tool from path: %s", path) + def _tool_file_on_disk(self, path: str) -> bool: + """Existence gate for a conf-referenced tool file. + + A per-tool stat dominates the walk of a large shed conf on a + network filesystem (CVMFS); the lazy toolbox overrides this to + answer from its index instead. + """ + return os.path.exists(path) + + def _missing_repository_log_level(self, path: str) -> int: + """Severity for a shed tool whose repository has no install-DB row. + + Warning by default — for the eager toolbox that usually means lost + install records. The lazy toolbox downgrades index-covered tools: + conf-provided repositories (a CVMFS shed conf) are absent from the + install database by design, and one warning per tool is thousands + of lines per boot there. + """ + return logging.WARNING + def get_tool_repository_from_xml_item( self, elem: "Element", path: str ) -> Union[ToolConfRepository, "ToolShedRepository"]: @@ -1087,7 +1107,7 @@ class AbstractToolBox(ManagesIntegratedToolPanelMixin): "Attempted to load tool shed tool, but the repository with name '%s' from owner '%s' was not found " "in database. Tool will be loaded without install database." ) - log.warning(msg, repository_name, repository_owner) + log.log(self._missing_repository_log_level(path), msg, repository_name, repository_owner) # Figure out path to repository on disk given the tool shed info and the path to the tool contained in the repo assert installed_changeset_revision repository_path = os.path.join( diff --git a/lib/galaxy/tools/lazy_toolbox.py b/lib/galaxy/tools/lazy_toolbox.py index 833fe1637d4..2ce91394ef5 100644 --- a/lib/galaxy/tools/lazy_toolbox.py +++ b/lib/galaxy/tools/lazy_toolbox.py @@ -496,6 +496,10 @@ class LazyToolBox(ToolBox): # via ``_rebuild_shed_short_id_map``. self._shed_short_id_to_guids: dict[str, set[str]] = {} self._store_watcher: ToolSourceStoreWatcher | None = None + # Identity-keyed cache of every indexed ``source_path`` — see + # ``_index_source_paths``. Set before ``super().__init__`` because + # the eager walk consults it through ``_tool_file_on_disk``. + self._index_source_paths_cache: tuple[ToolIndex, set[str]] | None = None # Eager init — its ``_init_tools_from_configs`` is overridden so the # walk goes through our ``create_tool`` seam and hands back LazyTool @@ -1008,6 +1012,55 @@ class LazyToolBox(ToolBox): self._rebuild_shed_short_id_map() return self._resolve_index_entry(config_file, guid) + def _index_source_paths(self) -> set[str]: + """Every ``source_path`` the current in-memory index covers. + + Cached against the identity of ``_tool_index``, so any reload + (which always assigns a fresh ``ToolIndex``) refreshes it without + reset plumbing. In-place entry removals can leave a stale extra + path here — that only skips one stat for a tool ``create_tool`` + will then resolve or self-heal, so it is harmless. + """ + index = self._tool_index + if index is None: + return set() + cached = self._index_source_paths_cache + if cached is not None and cached[0] is index: + return cached[1] + paths: set[str] = set() + for versions in index.entries_by_version.values(): + for entry in versions.values(): + if entry.source_path: + paths.add(entry.source_path) + for entry in index.entries.values(): + if entry.source_path: + paths.add(entry.source_path) + self._index_source_paths_cache = (index, paths) + return paths + + def _tool_file_on_disk(self, path: str) -> bool: + """Answer the eager walk's existence gate from the index. + + The populator established existence when it stored the source, and + materialisation reads the raw source from the store, not the file — + so for an index-covered tool the per-tool stat proves nothing and, + on a CVMFS-resident shed conf, costs minutes of boot time. + """ + if os.path.abspath(path) in self._index_source_paths(): + return True + return os.path.exists(path) + + def _missing_repository_log_level(self, path: str) -> int: + """Index-covered shed tools expectedly lack install-DB rows. + + A conf-provided repository (CVMFS shed conf) is never installed + through the install database, so the eager warning would repeat + for every one of its tools on every boot. + """ + if os.path.abspath(path) in self._index_source_paths(): + return logging.DEBUG + return logging.WARNING + def _resolve_index_entry(self, config_file, guid: str | None) -> ToolIndexEntry | None: """Find a matching index entry for the (config_file, guid) pair, or ``None``.""" if self._tool_index is None: diff --git a/test/unit/app/tools/test_lazy_tool.py b/test/unit/app/tools/test_lazy_tool.py index 44463a49d29..7a042e81d0b 100644 --- a/test/unit/app/tools/test_lazy_tool.py +++ b/test/unit/app/tools/test_lazy_tool.py @@ -509,3 +509,35 @@ def test_remove_tool_by_id_broadcasts_reload_to_peers(monkeypatch): box._store.remove_index_entry.assert_called_once_with("doomed") assert calls == [("reload_tool_source_cache", {"noop_self": True})] assert "doomed" not in box._tools_by_id + + +def test_tool_file_on_disk_answers_from_index(tmp_path): + box = _seam_box() + box._index_source_paths_cache = None + indexed_path = "/cvmfs/nowhere.example.org/shed_tools/repos/o/n/rev/t1.xml" + box._tool_index.add_entry(_entry(id="t1", source_path=indexed_path)) + assert box._tool_file_on_disk(indexed_path) is True + assert box._tool_file_on_disk(str(tmp_path / "missing.xml")) is False + on_disk = tmp_path / "real.xml" + on_disk.write_text("") + assert box._tool_file_on_disk(str(on_disk)) is True + + +def test_missing_repository_log_level_downgrades_indexed_paths(): + box = _seam_box() + box._index_source_paths_cache = None + indexed_path = "/cvmfs/nowhere.example.org/shed_tools/repos/o/n/rev/t1.xml" + box._tool_index.add_entry(_entry(id="t1", source_path=indexed_path)) + assert box._missing_repository_log_level(indexed_path) == logging.DEBUG + assert box._missing_repository_log_level("/elsewhere/t2.xml") == logging.WARNING + + +def test_index_source_paths_refresh_on_index_swap(): + box = _seam_box() + box._index_source_paths_cache = None + box._tool_index.add_entry(_entry(id="t1", source_path="/a/t1.xml")) + assert "/a/t1.xml" in box._index_source_paths() + swapped = ToolIndex() + swapped.add_entry(_entry(id="t2", source_path="/b/t2.xml")) + box._tool_index = swapped + assert box._index_source_paths() == {"/b/t2.xml"}