Skip to content

fix: harden MCP against DB corruption crashes and orphaned workers#37

Open
stashilin wants to merge 16 commits into
intuit:mainfrom
shilin23061991:fix/mcp-crash-resilience
Open

fix: harden MCP against DB corruption crashes and orphaned workers#37
stashilin wants to merge 16 commits into
intuit:mainfrom
shilin23061991:fix/mcp-crash-resilience

Conversation

@stashilin

Copy link
Copy Markdown

Что чинит

Три причины нестабильности MCP-сервера, наблюдавшиеся вживую (SIGSEGV-краши, orphan-worker'ы с PPID=1, держащие mcp.lock):

  1. Preflight-валидация файла БД перед kuzu::Database::new — обрезанный/битый файл теперь возвращает Err вместо нативного abort/segfault всего процесса (graph и docs store).
  2. Crash recovery включает startup cwd даже при пустом registry — раньше auto-reindex был no-op.
  3. Worker следит за супервизором (INFIGRAPH_SUPERVISOR_PID) и завершается после его смерти: getppid-reparent-проверка на Unix (иммунна к PID reuse), OpenProcess/WaitForSingleObject на Windows.

Ревью

4 раунда CodeRabbit CLI, все находки (2 major, 3 minor, 2 trivial) исправлены, финальный прогон чистый.

Проверено

  • E2E: SIGKILL супервизора → worker вышел <8 сек; битый graph → wipe/rebuild без краша
  • fmt/clippy чистые, целевые тесты зелёные (--test-threads=1 для известных параллельных флейков)
  • Windows-код проверен cargo check --target x86_64-pc-windows-msvc

stashilin and others added 16 commits July 25, 2026 17:20
- VoyageEmbedder (voyage-code-3, 256-dim) activated by VOYAGE_API_KEY,
  preferred over Model2Vec in the embedder factory
- rerank module: Cohere rerank-v3.5 via COHERE_API_KEY, best-effort
  second stage for the MCP search tool with fallback to local ranking

Amp-Thread-ID: https://ampcode.com/threads/T-019f9980-7174-7219-8f5c-3f70ad45231a
Co-authored-by: Amp <amp@ampcode.com>
OLLAMA_API_KEY selects the Ollama chat API (cloud or self-hosted),
falling back to Anthropic. Model/base URL/max tokens overridable via
INFIGRAPH_LLM_* env vars.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Move Voyage/Cohere/Ollama implementations into dedicated modules so
upstream files carry only short dispatch hooks (~19 lines total):

- embed/voyage.rs: VoyageEmbedder (was inline in embed/mod.rs)
- rerank.rs: maybe_rerank() single-call hook for Cohere second-stage
  rerank (was a 36-line block in mcp tools/search.rs)
- review/ollama.rs: Ollama review backend (was inline in review/llm.rs)

Behavior unchanged: providers activate only via VOYAGE_API_KEY /
COHERE_API_KEY / OLLAMA_API_KEY; without keys everything is fully
upstream-compatible and offline. Eases future rebases onto upstream.

Amp-Thread-ID: https://ampcode.com/threads/T-019f9980-7174-7219-8f5c-3f70ad45231a
Co-authored-by: Amp <amp@ampcode.com>
…view

- concatenate all Anthropic text blocks instead of reading only the first
  (reasoning models emit a thinking block before text); add unit tests
- Ollama provider no longer hijacks the backend when ANTHROPIC_API_KEY is
  set; explicit INFIGRAPH_LLM_PROVIDER=ollama override supported

Amp-Thread-ID: https://ampcode.com/threads/T-019f9980-7174-7219-8f5c-3f70ad45231a
Co-authored-by: Amp <amp@ampcode.com>
- voyage: strict response parsing (index/dim/count validation), embedder identity
- embed: provider identity sidecar; missing record is fail-safe stale;
  identity() is now mandatory per provider (trigram/model2vec/voyage distinct)
- rerank: 30s request timeout; response must rank every document (top_n = doc count)
- review/llm: preserve raw content blocks (thinking) on max_tokens continuation
- review/ollama: OLLAMA_API_KEY optional for self-hosted; default model glm-5.2:cloud
- install-local.sh: strict arg parsing; stage both binaries before atomic rename

Amp-Thread-ID: https://ampcode.com/threads/T-019f9980-7174-7219-8f5c-3f70ad45231a
Co-authored-by: Amp <amp@ampcode.com>
- preserve grep correlation boost through Cohere rerank and search
  escalation (maybe_rerank now reports whether scores were replaced;
  boost re-applied per grep match, then re-sorted before truncate)
- Voyage embedder: bounded request timeout via shared ureq agent,
  reject non-finite embedding values after f32 narrowing
- embed build: propagate embed_batch errors instead of silently
  persisting a partial index and provider identity
- install-local.sh: actually validate the staged infigraph-mcp binary
  (runs with stdin at EOF so the server exits immediately)
- tests: dimension-switch staleness, non-finite Voyage values,
  maybe_rerank not-applied contract

Amp-Thread-ID: https://ampcode.com/threads/T-019f9980-7174-7219-8f5c-3f70ad45231a
Co-authored-by: Amp <amp@ampcode.com>
- maybe_rerank returns Option<reranked-prefix-len>; grep boost is
  re-applied and re-sorted within the reranked prefix only, so Cohere
  [0,1] scores never mix with local-scale tail scores
- cohere_rerank: enforce and document top_n == documents.len() contract
- Ollama review: bounded request timeout, strict response validation
  (missing message.content/done_reason now error instead of silently
  passing), extract_json finds the first valid JSON object instead of
  a first-{-to-last-} slice
- embed build: remote providers (Voyage) run batches sequentially to
  avoid parallel rate-limit failures; local models keep the rayon pool
- install-local.sh: correct the atomic-replacement claim (per-binary
  atomic, pair is not)

Amp-Thread-ID: https://ampcode.com/threads/T-019f9980-7174-7219-8f5c-3f70ad45231a
Co-authored-by: Amp <amp@ampcode.com>
- Validate graph/docs DB file size before kuzu::Database::new to turn
  truncated-file native aborts into recoverable Err
- Include startup cwd in MCP crash recovery even when registry is empty
- Worker now monitors supervisor PID (INFIGRAPH_SUPERVISOR_PID) and
  exits when the parent dies instead of lingering with PPID=1 holding
  mcp.lock

Amp-Thread-ID: https://ampcode.com/threads/T-019f9c12-ebeb-75a1-a4f5-d05903800d7d
Co-authored-by: Amp <amp@ampcode.com>
- lifecycle: use getppid() reparent check for direct-child workers so
  supervisor PID reuse can't keep an orphan alive; process_alive stays
  as fallback for non-direct-child debug setups
- store: propagate non-NotFound metadata errors in validate_db_file
  instead of masking them as fresh-create
- recovery: require .infigraph to be a directory; add regular-file test
  and exercise canonical-path dedup with a syntactic alias

Amp-Thread-ID: https://ampcode.com/threads/T-019f9c12-ebeb-75a1-a4f5-d05903800d7d
Co-authored-by: Amp <amp@ampcode.com>
kill(0, 0) signals the caller's whole process group and out-of-pid_t
values wrap negative (group/broadcast semantics) — treat both as
not-a-live-process instead.

Amp-Thread-ID: https://ampcode.com/threads/T-019f9c12-ebeb-75a1-a4f5-d05903800d7d
Co-authored-by: Amp <amp@ampcode.com>
…bject

process_alive previously returned true unconditionally on non-Unix, so
a Windows worker could never reap itself after supervisor death. Use
OpenProcess(PROCESS_SYNCHRONIZE) + zero-timeout WaitForSingleObject;
inaccessible-but-existing processes still count as alive so a healthy
worker is never killed spuriously. Verified against x86_64-pc-windows-msvc
with cargo check (API-level); behavior exercised by CI's Windows runners.

Amp-Thread-ID: https://ampcode.com/threads/T-019f9c12-ebeb-75a1-a4f5-d05903800d7d
Co-authored-by: Amp <amp@ampcode.com>

@murari316 murari316 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: hold — CI-workflow/secrets risk + unrelated scope bundled in

Thanks for tackling this — the crash-resilience mechanics themselves (DB preflight
validation, crash-recovery path inclusion, supervisor-watch) are sound engineering and
directly target invariants already documented in this repo's own CLAUDE.md. But this PR
can't be merged as-is for reasons independent of that code quality.

Blocking: CI workflow change

.github/workflows/ci.yml and .github/workflows/pages.yml add:

optimize_ci:
  uses: shilin23061991/common-actions/.github/workflows/optimize-ci.yml@main
  secrets:
    graphite_token: ${{ secrets.GRAPHITE_TOKEN }}

and gate fmt/check/build behind needs: optimize_ci + if: needs.optimize_ci.outputs.skip == 'false'.

Concerns:

  • The reusable workflow lives in a personal fork (shilin23061991/common-actions), not
    an org-owned repo, and is referenced unpinned (@main) — its behavior can change after
    review/merge with no diff ever appearing in this repo's history.
  • It receives secrets.GRAPHITE_TOKEN.
  • If that workflow's skip output is ever 'true', the required checks (fmt, check,
    build) don't run — and a skipped required check typically still satisfies branch
    protection. That means CI could report green having validated nothing.

This needs to be dropped, or replaced with a pinned (commit-SHA), org-owned equivalent,
before anything else here can be reviewed in good faith.

Unrelated scope in the same diff

Confirmed new to this branch (not on main):

  • .env.1password — references COHERE_API_KEY, KIMI_API_KEY, OLLAMA_API_KEY, VOYAGE_API_KEY
  • crates/infigraph-core/src/rerank.rs (new) — Cohere Rerank integration
  • crates/infigraph-core/src/embed/voyage.rs (new) — Voyage AI embedder
  • crates/infigraph-core/src/review/ollama.rs changes — Ollama backend wiring
  • scripts/install-local.sh, branding assets under branding-system//docs/assets/branding/

None of this relates to DB-corruption/process-supervision hardening. Please split this
into: (1) the crash-resilience fix alone, (2) the CI/tooling changes (if still wanted,
after addressing the concern above, as its own reviewable PR), (3) the new
provider integrations, as their own PR(s).

On the crash-resilience fix itself (once isolated)

Reviewed graph/store.rs/docs/store.rs (DB preflight size check), recovery.rs
(crash-recovery candidate inclusion), and lifecycle.rs (supervisor-watch) against
main and this repo's own documented invariants:

  • DB preflight validation: correctly implemented, new (confirmed via git log -S on
    main), and the size floor targets the actual crash mode (gross truncation). Worth
    noting in the description that this mitigates truncation specifically, not all
    corruption — a file just over the size floor with a corrupted header past the first
    page can still reach kuzu::Database::new unmodified.
  • Crash recovery + empty registry: correctly gated — every candidate path, including
    the new startup-cwd one, still requires .infigraph/ to exist before being wiped. Does
    not widen what counts as "corruption," only which already-indexed paths get considered.
  • Supervisor-watch: the Unix direct-child path (getppid() reparent check) is
    genuinely PID-reuse-immune as claimed. The non-direct-child fallback and the entire
    Windows path (OpenProcess/WaitForSingleObject) are not — and the PR description
    doesn't scope that distinction. Also: the getppid()-based reparent check (the actual
    headline mechanism) and the whole Windows path have zero test coverage — only the
    process_alive primitive is tested, Unix-only. Please add a test for
    spawn_parent_monitor's reparent path, and clarify the PID-reuse-immunity claim to the
    Unix direct-child case specifically.

Recommendation: request changes. Split the PR; drop or fix the CI-workflow change;
add the missing supervisor-watch test; then this is close to mergeable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants