fix(mcp): improve diagnostics, daemon resilience, and chart UX - #243
Open
StefanSteiner wants to merge 29 commits into
Open
fix(mcp): improve diagnostics, daemon resilience, and chart UX#243StefanSteiner wants to merge 29 commits into
StefanSteiner wants to merge 29 commits into
Conversation
A lock conflict on a caller-supplied .hyper file (SQLSTATE 55006, or a legacy "already attached"/"file is locked" phrase from older hyperd) previously reached the user via Engine::execute_command, whose generic From<Error> conversion leaves 55006 as SqlError. Only the reserved persistent-attach path reclassified it to RESOURCE_BUSY. Add Engine::execute_attach_command plus the shared is_attach_lock_conflict predicate and attach_lock_error mapper (mirroring persistent_attach_error), and route AttachRegistry's ATTACH through it so a held file surfaces as RESOURCE_BUSY with the effective path, preserved diagnostic/SQLSTATE, and doctor-oriented recovery guidance. Unrelated 55006 errors keep their generic mapping.
parse_hex_color stripped an optional '#', checked s.len() == 6, then sliced s[0..2]/[2..4]/[4..6]. A six-byte string whose bytes are not all ASCII (e.g. "1é234", where é is two UTF-8 bytes) has len() == 6 but its byte offsets do not land on char boundaries, so the slice panicked and aborted the chart request. Guard with !s.is_ascii() before slicing; ASCII guarantees one byte per char, so every slice is a char boundary. Adds regression coverage for the bare and '#'-prefixed six-byte non-ASCII forms.
In ephemeral-only mode (no persistent database) doctor_client_log_path falls back to resolve_log_dir(None), which keys the log directory to the doctor process's own PID. A separately running MCP server logs under its own per-process directory, so the reported path can never correspond to a real session. Emit an 'ephemeral_client_log_path_illustrative' warning so the report states this instead of presenting the path as authoritative.
The same-directory fallback branch of findBinary() reused sourcePackagePath, which points at the platform *subdirectory*'s package.json — a path that does not exist in this branch (the manifest sits next to the binary in __dirname). launcher_info.platform.package_path therefore reported a nonexistent file. Recompute it as join(__dirname, 'package.json') so launcher diagnostics report the manifest that is actually present.
… on slow CI The non-blocking health listener slept 100ms between accept() attempts on the WouldBlock path. The doctor network phase budgets only a few hundred ms for a STATUS round-trip; on slow CI runners (macos-14 has ~3 CPUs) a 100ms idle gap between accepts could push the accept past that window, failing real_health_listener_accept_cadence_fits_doctor_budget. Drop the idle sleep to 5ms — well within the doctor budget and the 650ms watchdog the doctor tests assert — without meaningfully raising idle CPU.
…tions std::fs::canonicalize prepends the \\?\ verbatim prefix on Windows, but the paths the doctor binary reports come from current_dir/current_exe, which are un-prefixed. The sandbox root and expected-executable assertions compared a prefixed expected path against an un-prefixed reported one and failed only on Windows CI. Add canonicalize_for_test (mirroring ingest.rs's prefix stripping, leaving genuine UNC paths alone) and use it for both. No-op on non-Windows.
cargo-audit fails on the yanked chacha20 0.10.0 pulled transitively via rand's ChaCha20 RNG. 0.10.2 is a semver-compatible patch (lockfile-only, no Cargo.toml or public-API change) that clears the yanked-crate advisory and turns the audit job green.
The [0.5.0] section had two sibling '### Fixed' headings (a stray single-bullet block above '### Added' plus the real one after '### Performance'), which markdownlint flags as MD024/no-duplicate-heading. Fold the stray NUMERIC-sign bullet into the canonical Fixed block, leaving proper Keep-a-Changelog ordering (Added -> Changed -> Removed -> Performance -> Fixed). Add a root .markdownlint.json enabling MD024 siblings_only so legitimately repeated section names (Added/Changed/Fixed) under different version headings no longer trip the rule. Together these clear all four MD024 warnings the editor reported on CHANGELOG.md.
The macos-14 CI runner (~3 cores, CPU-saturated) intermittently failed `real_health_listener_accept_cadence_fits_doctor_budget`: doctor reported the daemon Missing instead of LiveFromScan. Root cause is the per-socket-operation ceiling, not the health-listener accept cadence. The earlier health.rs WouldBlock poll reduction (100ms->5ms) was necessary hygiene but not sufficient here: the doctor's connection lands in the listen backlog immediately, so its PONG/STATUS read — not the accept poll — is the binding constraint. When a daemon is slow to start servicing its accept loop, that read exceeded the old 150ms DOCTOR_NETWORK_PHASE_TIMEOUT and doctor timed out. Raise DOCTOR_NETWORK_PHASE_TIMEOUT 150ms->300ms (the actual fix, ~2x the observed CI slack) and DOCTOR_DAEMON_TIMEOUT 400ms->500ms (the global deadline), with rationale doc-comments. On success doctor returns as soon as STATUS arrives, so the widened waits only cost latency when a daemon is truly absent; 500ms stays comfortably under the 650ms watchdog the drip test asserts.
…indows `persistent_lock_keeps_mcp_available` asserted that the RESOURCE_BUSY diagnostic names the exact effective persistent path via `diagnostic.contains(effective_path)`. The diagnostic is the JSON-serialized error text, so on Windows the path's backslashes are JSON-escaped (`\` -> `\\`) and the raw un-escaped path never matches — a deterministic Windows-only failure (Unix paths use `/`, which JSON does not escape, so it passed there). Compare against the JSON-escaped form of the path instead. On Unix the path has no backslashes, so the escaped form equals the raw path and the assertion is unchanged; on Windows it now matches the escaped backslashes in the JSON. Mirrors the existing `engine_tests.rs` sibling, which already dodges this by asserting against the parsed `error.message` field rather than the JSON text.
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
1 participant
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.
Overview
This PR is a focused HyperDB MCP UX, diagnostics, and resilience release. It preserves the public Rust structs and the existing 33-tool surface while making installation/runtime state easier to diagnose, routed results more explicit, daemon handling safer, and charts more useful for operational analysis.
Diagnostics, installation, and daemon identity
hyperdb-mcp doctorcommand with human and JSON output. It reports installation identity, paths, persistent database state, daemon discovery/health, and actionable warnings without starting Hyper, creating state, or mutating the database.STATUSrecords with additive build identity while preserving the publicDaemonInfoshape and compatibility with legacy discovery files.Hyperd lifecycle and recovery
Database routing and error semantics
resolved_databasemetadata to query, execute, data, KV, and copy success results so clients can tell whether work used local, persistent, or an attached database.55006asRESOURCE_BUSYonly at the reserved persistent attachment boundary; unrelated SQL failures keep their existing mapping and raw diagnostics.Status, resources, and catalog contract
statusidentity data, including MCP/API versions, installation facts, default database, busy state, watcher state, attachment state, and read-only mode.hyper://workspacecompatibility resources to describe the separate local and persistent database model rather than a collapsed workspace mode.doctorfrom MCP tools, and measures the typed catalog payload against its 57,344-byte budget.Chart improvements
Documentation and compatibility
HYPERD_PATHresolution, foreground daemon behavior, read-only guardrails, cross-database routing, chart behavior, Hyper export side effects, and doctor limitations.ChartOptions,render_chart, andDaemonInfosource-compatible; no version, manifest, or lock-file change is included.Validation
Completed validation includes:
cargo fmt --all --checkHYPERD_PATH=/Users/ssteiner/dev/bin/hyperd cargo test -p hyperdb-mcp— 583 passed, 0 failed, 16 ignoredHYPERD_PATH=/Users/ssteiner/dev/bin/hyperd cargo test --workspace --exclude hyperdb-api-node --exclude hyperdb-bootstrap— 1,546 passed, 0 failed, 38 ignorednode --test hyperdb-mcp/npm/bin.test.js— 6 passednode --check hyperdb-mcp/npm/bin.jsFollow-up
#242 tracks an intentionally separate resilience improvement: recognizing and recovering a
hyperdprocess that remains alive but is completely unresponsive (for example, afterSIGSTOP).Attribution
AI-assisted by ChatGPT 5.6 Sol and Terra.