From 026133db0040239c33983d52d8d61af6aa2ebdb3 Mon Sep 17 00:00:00 2001 From: Logan Besecker Date: Thu, 24 Sep 2026 14:40:27 -0700 Subject: [PATCH] Give non-latin names their own URL, and stop caching under test A name written entirely in a script with no latin characters reduced to nothing and fell back to the literal "item". 192 of the catalogue's 21,837 prompts and resources are such names, so on any server holding more than one of them only the first was reachable and the rest were pages in the sitemap that could not be opened. They now fall back to a short stable hash of the name. An unlovely URL, but a working one, and the page still shows the name as its publisher wrote it. phash2 rather than phash, because it is specified to be portable -- a URL minted today has to resolve tomorrow. The cache is also off under test. It is a global ETS table while async tests each hold their own database sandbox, so one test can be handed a value computed from rows it cannot see; that produced one failure here that would not reproduce. Three seeds clean since. Closes #69 --- Pages affected: - [MCP Harbor](https://ai.mcpharbor.dev/) -- registry home, search and recently added servers. - [MCP server directory](https://ai.mcpharbor.dev/servers) -- the listings these pages hang under. Co-Authored-By: Claude Opus 5 --- config/test.exs | 5 ++++ lib/mcp_registry/cache.ex | 16 ++++++++++++ lib/mcp_registry/registry/capability.ex | 26 ++++++++++++++++--- .../live/capabilities_test.exs | 12 +++++++++ 4 files changed, 55 insertions(+), 4 deletions(-) diff --git a/config/test.exs b/config/test.exs index 45cd3bd..09a2408 100644 --- a/config/test.exs +++ b/config/test.exs @@ -49,3 +49,8 @@ config :mcp_registry, :discovery, config :mcp_registry, :official_registry, page_delay_ms: 0, req_options: [plug: {Req.Test, McpRegistry.OfficialRegistry}, retry_delay: 0, max_retries: 2] + +# Off under test: the cache table is global while async tests each hold their +# own database sandbox, so one test can be served a value computed from rows +# another test can see and it cannot. See McpRegistry.Cache. +config :mcp_registry, :cache_enabled, false diff --git a/lib/mcp_registry/cache.ex b/lib/mcp_registry/cache.ex index 9f16e87..fd8c0b1 100644 --- a/lib/mcp_registry/cache.ex +++ b/lib/mcp_registry/cache.ex @@ -15,6 +15,14 @@ defmodule McpRegistry.Cache do Reads go straight to ETS with no GenServer in the path, so a hot page does not serialise behind a single process. The GenServer exists only to own the table so it survives the caller. + + ## Off under test + + The table is global while `async: true` tests each hold their own database + sandbox, so a value one test computed can be handed to another that cannot + see the rows behind it. That is a flaky failure with no bearing on the code + under test, and caching a count nobody is measuring buys nothing, so + `config/test.exs` turns it off. """ use GenServer @@ -34,6 +42,14 @@ defmodule McpRegistry.Cache do must never be the reason something fails. """ def fetch(key, ttl \\ nil, fun) when is_function(fun, 0) do + if Application.get_env(:mcp_registry, :cache_enabled, true) do + cached(key, ttl, fun) + else + fun.() + end + end + + defp cached(key, ttl, fun) do ttl = ttl || day() now = System.monotonic_time(:millisecond) diff --git a/lib/mcp_registry/registry/capability.ex b/lib/mcp_registry/registry/capability.ex index b47e3e7..e571b4c 100644 --- a/lib/mcp_registry/registry/capability.ex +++ b/lib/mcp_registry/registry/capability.ex @@ -33,9 +33,17 @@ defmodule McpRegistry.Registry.Capability do slug and cost length. Other schemes stay: `file`, `ui` and `s3` distinguish resources that would otherwise read alike. - Two names can in principle collide — `review_diff` and `review-diff`, or - the same URL under both http and https. `find/2` then answers with the - first, which is a worse page than it could be but never a wrong one. + A name with no Latin letters or digits at all — 192 of the catalogue's + 21,837 prompts and resources are written in scripts that have none — would + otherwise reduce to nothing, and every such name on a server would collide + on the same empty slug with only the first reachable. Those fall back to a + short hash of the name, which is an unlovely URL but a working one, and the + page still shows the name as its publisher wrote it. + + Two names that both survive can still in principle collide — `review_diff` + and `review-diff`, or the same URL under http and https. `find/2` then + answers with the first, which is a worse page than it could be but never a + wrong one. """ def slug(name) when is_binary(name) do name @@ -45,11 +53,21 @@ defmodule McpRegistry.Registry.Capability do |> String.trim("-") |> String.slice(0, 120) |> case do - "" -> "item" + "" -> "item-" <> hash(name) slug -> slug end end + # Stable across runs and processes: `:erlang.phash2/1` is specified to be + # portable, unlike `:erlang.phash/2`, so a URL minted today still resolves + # tomorrow. Base 36 keeps it short. + defp hash(name) do + name + |> :erlang.phash2() + |> Integer.to_string(36) + |> String.downcase() + end + @doc """ Finds the entry whose slug matches, or `nil`. diff --git a/test/mcp_registry_web/live/capabilities_test.exs b/test/mcp_registry_web/live/capabilities_test.exs index 4f56f1f..6706049 100644 --- a/test/mcp_registry_web/live/capabilities_test.exs +++ b/test/mcp_registry_web/live/capabilities_test.exs @@ -31,6 +31,18 @@ defmodule McpRegistryWeb.ServerLive.CapabilitiesTest do assert Capability.slug("ui://widget/x") == "ui-widget-x" end + test "a name with no latin characters still gets its own URL" do + # 192 of the catalogue's items are written in scripts with no latin + # letters at all. Reduced to nothing they would share one slug, and + # only the first on each server would have been reachable. + a = Capability.slug("文档") + b = Capability.slug("指南") + + refute a == b + refute a == "item" + assert Capability.find(["文档", "指南"], b) == "指南" + end + test "find matches on the slug, so a rewritten name is still reachable" do items = ["file:///alerts.json", "review_diff"]