Repository navigation
fix: repair stale update-available notice after a successful gateway.update_server - #147
Conversation
d33eaec to
24c3c4a
Compare
…nned servers (board review round 2)
Board review round 3 finding 2: detect_package_type strips the :tag off a docker image reference, so "docker run acme/server:1.2.3" would pull acme/server:latest, restart the still-pinned 1.2.3 config, and record the new digest as active -- the same silent misreporting the npm pin gate already prevented. Factors the docker image scan out of detect_package_type (_docker_image_arg) so pin detection and package detection cannot disagree, adds _docker_image_tag (final path segment only, so registry:5000/img is not misread as a tag of 5000), and covers cargo's separate --version pin.
|
Round-3 board reconciliation. Docker/cargo pin gap fixed here ( Verified independently by the reviewer, not relayed:
|
Ships the gateway.update_server repair from #147: the tool now actually restarts the target server (refresh() is a diff-based reconcile that never respawned an argv-unchanged process, so the OLD package kept serving), gates every cache/notice side effect on that restart succeeding, resolves the probe and the restart through one config resolver so a .mcp.json override cannot diverge from the manifest, and refuses version-pinned servers across npm/uvx/docker/cargo instead of silently recording an update that cannot take effect. Note for callers: gateway.update_server gains an optional force parameter (default false) that cancels the target's pending requests before the restart, and can now return ok=false when the package was fetched but the server could not be restarted.
Post-merge board record: claude/correctness seat filled nativelyThe 4th seat on the review board ( How it was filled, for the record. The request was built through the runtime's own What the seat verified independently, against source at the merge commit rather than from the bundle narrative:
It also caught a defect in the review process, not the code. The round-3 bundle handed to the board was not the diff that shipped: That directly explains the standing Nothing here changes the merged result. #150 and #151 remain open and correctly non-blocking. |
Root cause
The maintainer ran
gateway.update_server(server_name="context7"). It reported success, but the gateway kept advertisingUpdate available for 'context7': ...on every subsequent call, and still did after a restart.update_servercallsgateway.refresh()to reconnect downstream servers after a package update.refresh()was not enough because it only reloads.mcp.jsonconfigs and reconnects transports — it never touches the descriptions cache. The "current version" shown in the stale notice comes fromself._descriptions_cache.servers[server_name].version(GeneratedServerDescriptions.version), which is an upstream-latest snapshot from the lastpmcp refresh/describe pass, not the installed version.update_serverhad no code path that ever rewrote it, so after a successful update the value stayed pinned at its pre-update snapshot forever.Three sites read that stale value and kept reproducing the identical notice:
_get_update_warning()(handlers.py)catalog_search'sstale_updateslist comprehension, which reads_stale_check_cachedirectly_run_stale_index()sweepThe old code's
self._stale_check_cache.pop(server_name, None)on success only cleared the memoized stale-check tuple — the very next call to any of the three sites above recomputed it from the same staledesc.versionand reproduced the same notice. A restart made it permanent, sinceload_descriptions_cachereads that same stale value back off disk.Fix
On
update_server's success path, using thelatest_versionit already fetches viaget_package_version:latest_versionis truthy and the server has a descriptions-cache entry, writelatest_versionintoself._descriptions_cache.servers[server_name].version(in place — this is the same objectGatewayServerholds;refresh()never replaces it, so the in-memory fix takes effect everywhere immediately).save_descriptions_cache, so the fix survives a restart, not just the current process.GatewayToolsdid not previously have access to the cache path, so I added adescriptions_cache_path: Path | None = Noneconstructor parameter (defaulting toNone, so all ~80 existing test call sites are unaffected) and wired it fromGatewayServer.__init__via the existingget_cache_path(self._cache_dir)helper. A disk-write failure is logged and swallowed — the package update itself already succeeded, sookstill reflects that, not the bookkeeping._stale_check_cacheis now repopulated with(now, latest_version, latest_version)instead of just popped, whenlatest_versionis truthy. This makescatalog_search'sstale_updates(which reads this cache directly, independent of the descriptions cache) agree the server is current immediately, and avoids a redundant network re-fetch for the full 6h TTL. Whenlatest_versionisNone(fetch failed), the oldpop()behavior is kept — there's nothing fresher to record.latest_versionisNone— leaving the old version is better than overwriting it with an unknown/failed value.ok=Falsereturn before this point) never reaches the version-bump code, so the recorded version is untouched on failure.Scope note
This fix introduces a side effect, not a pre-existing one. Bumping
desc.versionon a successful update changes what a laterrefresh_serverstaleness check (src/pmcp/manifest/refresher.py:213-223) does:update_server,desc.versionstayed at its pre-update snapshot (e.g.2.1.2) while upstream was4.0.x.is_version_newer(2.1.2, 4.0.x)->True-> a plainpmcp refreshwould regenerate that server's cached tool descriptions.desc.versionis now4.0.ximmediately.is_version_newer(4.0.x, 4.0.x)->False-> the staleness check's early return fires ->pmcp refreshskips regenerating that server's tool descriptions, even if the update shipped different tools.So the stale version was accidentally also acting as the trigger that got tool descriptions regenerated on the next refresh, and this PR removes that trigger for the updated server. Blast radius is bounded and non-blocking:
handlers.pyskips any server thatis_server_online()-- "live tools take precedence"). A server that's online right after its own update is unaffected.refresh_all()only runs unconditionally at startup when no cache exists yet (server.py:783).pmcp refresh --force(which bypasses the staleness check).Filed as #148, with the before/after mechanism, blast-radius bounds, and a candidate fix (regenerate descriptions directly from the tool list
update_serveralready has post-reconnect, instead of relying on a laterrefresh_serverpass). Not implemented here -- out of scope for this PR.Verification
RED check (required by this repo's testing standard — a passing test is not evidence a test works): reverted just the fix (
src/pmcp/tools/handlers.py+src/pmcp/server.py, tests untouched) and reran the 6 new tests. 4 of the 6 went RED as expected (the 2 failure-path tests stayed green because those paths were already correct pre-fix):Note on
test_update_server_clears_catalog_search_stale_updates: a naive version of this test would pass even without the fix, because the old code's own_stale_check_cache.pop()already silences an immediatecatalog_search. To actually discriminate the bug, the test calls_get_update_warning()betweenupdate_server()andcatalog_search()to force the same recompute production traffic would trigger — that's what turned it red above.Restored the fix, reran: all 6 green.
Gates (from inside the worktree,
uv run):pytest -q: 2527 passed, 3 skipped, 0 failed (baseline onmainwas 2521 passed / 3 skipped; +6 = my new tests, nothing else changed)ruff check .: all checks passedruff format --check .: my 3 touched files (src/pmcp/tools/handlers.py,src/pmcp/server.py,tests/test_tools.py) are clean; 2 pre-existing unformatted files underdiagnostics/issue-79-1b/are unrelated and unchanged by this PR (confirmed present onmainbefore this branch)mypy src:Success: no issues found in 42 source filesScope discipline
Fixed this bug only. Did not touch
_version_cacheinversion_checker.py, add TTLs, or refactor the version-checking subsystem.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is enabled.