fix(discovery): prune stale entries by pid liveness, not shared-port reachability#45
Merged
Merged
Conversation
…reachability victauri-test's discovery inferred a server was alive purely from TCP reachability of its advertised port. But every Victauri app registers on the same default port (7373), so a crashed app's stale <pid> entry still looked "reachable" — a DIFFERENT live app now holds 7373 — and was never pruned. Its stale token then collided with the live server's in `unique_token_for_port` => None => `victauri check/invoke/ coverage` return **401 Unauthorized** on any machine that has run more than one Victauri app. (The MCP-bridge discovery path already gated on the pid and so worked; this brings the test/CLI path in line.) Fix: gate liveness on the owning PID — a dead owner is stale even when the shared port answers, so the entry is pruned and token selection stays unambiguous. Also resolves `victauri doctor` reporting "server not running" in this case. - find_live_servers -> testable find_live_servers_in(base, is_alive, is_reachable) + cross-platform is_process_alive (tasklist / kill -0), mirroring victauri-cli. - diagnose_discovery gates Live/Stale on pid liveness too. - Regression test: two entries on the shared port (dead + live) -> only the live token selected, dead dir pruned. 115 lib tests pass, clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W3W4WofCBkV9TgYAzjTdM7
Collaborator
Author
|
Note on the two red checks — pre-existing, not from this PR. This change touches a single file (
The checks that validate this change are Check & Lint (fmt/clippy/ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
victauri check/invoke/coveragereturn 401 Unauthorized against a running, auth-enabled app on any machine that has run more than one Victauri app.Root cause:
victauri-test's discovery (find_live_servers) inferred a server was alive purely from TCP reachability of its advertised port. But every Victauri app registers on the same default port (7373). So when a previous app crashes/exits, its stale<pid>/discovery dir still advertises 7373 — and a different live app now holding 7373 makes that port probe succeed. The dead entry is never pruned, and its stale token is added as a "live server". Two entries then match port 7373, sounique_token_for_portreturnsNone(ambiguous) → no token sent → 401.This is why the MCP-bridge path (
victauri-cli::bridge, which already gates on the pid) worked whilecheck/invoke/coveragedidn't — the exact asymmetry.The fix
Gate liveness on the owning pid, not just port reachability: a dead owner is stale even when the shared port answers, so the entry is pruned and token selection stays unambiguous.
find_live_servers→ testablefind_live_servers_in(base, is_alive, is_reachable)+ cross-platformis_process_alive(tasklist/kill -0), mirroring the audited helper invictauri-cli::bridge.diagnose_discoverygatesLive/Staleon pid liveness too (accurate classification on a shared port).victauri doctorreporting "server not running" in this case (it was the same faileddiscover()).Tests
New regression test: two discovery entries on the shared port (one dead, one live) → only the live token is selected and the dead dir is pruned — the exact 401 scenario, now green. Plus a mid-rebuild case (live owner, unreachable port → pruned).
cargo test -p victauri-test115/0,cargo clippyclean (deny-level workspace lints), rustfmt clean, and the repo pre-push gate passed.Notes / not in scope
%TEMP%\victauri\<pid>entries all on 7373 caused the 401; pruning dead-pid dirs fixed it. Discovered via the stale installed CLI too (0.5.6) — unrelated; upgrading is the separate fix.🤖 Generated with Claude Code