tools: prefer the exact source-path row when materialising from the store

lib/galaxy/tools/__init__.py

@@ -14,6 +14,7 @@ from collections.abc import (
     MutableMapping,
     Sequence,
 )
+from datetime import datetime
 from pathlib import Path
 from typing import (
     Any,
@@ -700,20 +701,22 @@ class ToolBox(AbstractToolBox):
         if store is None:
             return None
 
-        stored = None
+        # Resolve by the on-disk file path first. ``source_path`` is recorded
+        # at populate time and the populator updates that per-path row in place,
+        # so a hit here is content-current and exact (no regex shortcut, no
+        # ``sources[0]`` fallback that could silently return a different tool's
+        # source).
+        stored = store.get_by_source_path(os.path.abspath(str(config_file)))
 
-        # Shed installs hand us the full guid; look up directly.
-        if tool_id:
+        # Shed installs hand us the full guid; fall back to it only when the
+        # path lookup misses. ``get_by_tool_id`` has no ORDER BY and the
+        # append-only store accumulates superseded content rows for the same
+        # tool_id, so pick the most recently stored one rather than an
+        # arbitrary row.
+        if stored is None and tool_id:
             sources = store.get_by_tool_id(tool_id)
             if sources:
-                stored = sources[0]
-
-        # Local tools have no tool_id at this point — resolve by the on-disk
-        # file path. ``source_path`` is recorded at populate time, so a hit
-        # here is exact (no regex shortcut, no ``sources[0]`` fallback that
-        # could silently return a different tool's source).
-        if stored is None:
-            stored = store.get_by_source_path(os.path.abspath(str(config_file)))
+                stored = max(sources, key=lambda s: (s.stored_at is not None, s.stored_at or datetime.min))
 
         if stored is None:
             return None

test/unit/app/tools/test_tool_source_store_selection.py

@@ -0,0 +1,92 @@
+from datetime import (
+    datetime,
+    timedelta,
+    timezone,
+)
+from types import SimpleNamespace
+from unittest.mock import MagicMock
+
+from galaxy import tools
+from galaxy.tools import ToolBox
+from galaxy.tools.source_store.interface import StoredToolSource
+
+
+def _stored(tool_id=None, source_path=None, raw="<tool/>", stored_at=None):
+    return StoredToolSource(
+        hash=f"{tool_id}:{source_path}:{raw}",
+        tool_source_class="XmlToolSource",
+        raw_source=raw,
+        tool_id=tool_id,
+        source_path=source_path,
+        stored_at=stored_at,
+    )
+
+
+def _box(store):
+    box = ToolBox.__new__(ToolBox)
+    box.app = SimpleNamespace(config=SimpleNamespace(use_lazy_toolbox=True), tool_source_store=store)  # type: ignore[assignment]
+    return box
+
+
+def _patch_get_tool_source(monkeypatch):
+    monkeypatch.setattr(tools, "get_tool_source", lambda raw_tool_source, tool_source_class: raw_tool_source)
+
+
+def test_path_row_preferred_over_tool_id_rows(monkeypatch):
+    _patch_get_tool_source(monkeypatch)
+    store = MagicMock()
+    store.get_by_source_path.return_value = _stored(tool_id="t1", source_path="/tools/t1.xml", raw="path_row")
+    store.get_by_tool_id.return_value = [_stored(tool_id="t1", raw="guid_row")]
+    box = _box(store)
+
+    assert box._get_tool_source_from_store("/tools/t1.xml", tool_id="t1") == "path_row"
+    store.get_by_tool_id.assert_not_called()
+
+
+def test_tool_id_fallback_when_path_misses(monkeypatch):
+    _patch_get_tool_source(monkeypatch)
+    store = MagicMock()
+    store.get_by_source_path.return_value = None
+    store.get_by_tool_id.return_value = [_stored(tool_id="t1", raw="guid_row")]
+    box = _box(store)
+
+    assert box._get_tool_source_from_store("/tools/t1.xml", tool_id="t1") == "guid_row"
+
+
+def test_tool_id_fallback_picks_latest_stored_at(monkeypatch):
+    _patch_get_tool_source(monkeypatch)
+    now = datetime.now(timezone.utc)
+    store = MagicMock()
+    store.get_by_source_path.return_value = None
+    store.get_by_tool_id.return_value = [
+        _stored(tool_id="t1", raw="old", stored_at=now - timedelta(hours=1)),
+        _stored(tool_id="t1", raw="new", stored_at=now),
+        _stored(tool_id="t1", raw="mid", stored_at=now - timedelta(minutes=30)),
+    ]
+    box = _box(store)
+
+    assert box._get_tool_source_from_store("/tools/t1.xml", tool_id="t1") == "new"
+
+
+def test_tool_id_fallback_prefers_dated_row_over_undated(monkeypatch):
+    _patch_get_tool_source(monkeypatch)
+    now = datetime.now(timezone.utc)
+    store = MagicMock()
+    store.get_by_source_path.return_value = None
+    store.get_by_tool_id.return_value = [
+        _stored(tool_id="t1", raw="dated", stored_at=now),
+        _stored(tool_id="t1", raw="undated", stored_at=None),
+    ]
+    box = _box(store)
+
+    assert box._get_tool_source_from_store("/tools/t1.xml", tool_id="t1") == "dated"
+
+
+def test_returns_none_when_lazy_toolbox_disabled():
+    store = MagicMock()
+    box = ToolBox.__new__(ToolBox)
+    box.app = SimpleNamespace(config=SimpleNamespace(use_lazy_toolbox=False), tool_source_store=store)  # type: ignore[assignment]
+
+    assert box._get_tool_source_from_store("/tools/t1.xml", tool_id="t1") is None
+    store.get_by_source_path.assert_not_called()
+    store.get_by_tool_id.assert_not_called()