mirror of
https://github.com/open-jarvis/OpenJarvis.git
synced 2026-08-14 08:52:06 +00:00
fix(memory): surface clear error when openjarvis_rust missing instead of silent no-op (#527)
Memory tools degraded silently and misleadingly when the mandatory
`openjarvis_rust` extension was absent from the *serving* venv:
- `POST /v1/memory/store` returned HTTP 200 `{"status":"stored","note":
"no backend available"}` and stored nothing (silent data loss).
- `POST /v1/memory/index` returned a generic "No memory backend available",
and the desktop frontend discarded the server `detail` and threw a blanket
"Failed to index path", blaming the path instead of the real cause.
- `GET /v1/memory/config` reported `backend_type: sqlite` even though no
backend could be constructed.
Root cause: `SQLiteMemory.__init__` calls `get_rust_module()` (which raises
ImportError by design — the Rust ext is mandatory, no Python fallback), and
`_get_memory_backend` swallowed that ImportError and returned `None`,
conflating "native extension missing" (a hard install error) with "memory
intentionally disabled" (benign). A chunking floor also silently dropped whole
short documents, and the installer never verified the extension imported from
the serving venv before writing its success marker.
Fix (no fake Python fallback — the Rust ext stays mandatory by design):
- Add `MemoryBackendUnavailable` + `RUST_MISSING_HINT` in tools/storage/_stubs.
`SQLiteMemory.__init__` translates the bridge ImportError into this clear,
actionable error ("run `uv run maturin develop ...`").
- `_get_memory_backend` distinguishes the two cases: a missing native ext
raises HTTP 503 with the actionable hint; a benign unconfigured backend
still returns `None` (graceful path preserved for search/stats).
- `/store` now returns 503 instead of a 200 silent no-op.
- `/config` reports `available: false` + `detail` instead of falsely claiming
a healthy `backend_type`.
- `/index` adds a `note` when `chunks_indexed == 0` so "indexed" never
silently means "stored nothing".
- chunk_text no longer drops an entire short document below `min_chunk_size`
(the floor only discards tiny *trailing* fragments now).
- Frontend `storeMemory`/`indexMemoryPath` surface the server `detail` instead
of blanket strings; `MemoryConfig` gains optional `available`/`detail`.
(Left the pre-existing `backend` vs `backend_type` mismatch untouched.)
- build-extension.sh verifies `import openjarvis_rust` succeeds in the serving
venv before writing the `extension-built` marker.
Regression tests: tests/server/test_api_routes.py::TestMemoryRustMissing mocks
`get_rust_module` to raise ImportError and asserts /store (503, not 200 no-op),
/index (actionable detail, not "Failed to index path"), and /config
(available:false) all surface the clear error; tests/memory/test_chunking.py
asserts short-only docs are kept while tiny trailing fragments are still
filtered.
Fixes #502
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
18d9897317
commit
bc9fa6b3c8
+22
-3
@@ -948,12 +948,31 @@ export interface MemoryStats {
|
||||
|
||||
export interface MemoryConfig {
|
||||
backend: string;
|
||||
// Set by the server when the native `openjarvis_rust` extension is missing,
|
||||
// so the UI can show the real cause instead of a healthy-looking config.
|
||||
available?: boolean;
|
||||
detail?: string | null;
|
||||
context_from_memory: boolean;
|
||||
context_top_k: number;
|
||||
context_min_score: number;
|
||||
context_max_tokens: number;
|
||||
}
|
||||
|
||||
/**
|
||||
* Extract the server's `detail` message from a failed JSON response so the UI
|
||||
* surfaces the real cause (e.g. "openjarvis_rust extension is not installed")
|
||||
* instead of a blanket fallback string (#502).
|
||||
*/
|
||||
async function memoryErrorDetail(res: Response, fallback: string): Promise<string> {
|
||||
try {
|
||||
const data = await res.json();
|
||||
if (data && typeof data.detail === 'string' && data.detail) return data.detail;
|
||||
} catch {
|
||||
// Non-JSON body — fall through to the generic message below.
|
||||
}
|
||||
return fallback;
|
||||
}
|
||||
|
||||
export async function getMemoryStats(): Promise<MemoryStats> {
|
||||
const res = await apiFetch(`/v1/memory/stats`);
|
||||
if (!res.ok) throw new Error('Failed to fetch memory stats');
|
||||
@@ -977,16 +996,16 @@ export async function storeMemory(content: string, metadata?: Record<string, unk
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
body: JSON.stringify({ content, metadata }),
|
||||
});
|
||||
if (!res.ok) throw new Error('Failed to store memory');
|
||||
if (!res.ok) throw new Error(await memoryErrorDetail(res, 'Failed to store memory'));
|
||||
}
|
||||
|
||||
export async function indexMemoryPath(path: string): Promise<{ chunks_indexed: number }> {
|
||||
export async function indexMemoryPath(path: string): Promise<{ chunks_indexed: number; note?: string }> {
|
||||
const res = await apiFetch(`/v1/memory/index`, {
|
||||
method: 'POST',
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
body: JSON.stringify({ path }),
|
||||
});
|
||||
if (!res.ok) throw new Error('Failed to index path');
|
||||
if (!res.ok) throw new Error(await memoryErrorDetail(res, 'Failed to index path'));
|
||||
return res.json();
|
||||
}
|
||||
|
||||
|
||||
@@ -28,6 +28,22 @@ fi
|
||||
|
||||
cd "$SRC_DIR"
|
||||
if uv run maturin develop -m "$MANIFEST" >>"$LOG" 2>&1; then
|
||||
# Verify the extension actually imports from THIS venv before declaring
|
||||
# success. `maturin develop` can report success while installing the .so
|
||||
# into a different venv than the one that runs the server, which leaves
|
||||
# memory silently broken at runtime (#502). Only the import check below
|
||||
# proves the serving venv can load it.
|
||||
if ! uv run python -c "import openjarvis_rust" >>"$LOG" 2>&1; then
|
||||
rc=$?
|
||||
{
|
||||
echo "build-extension.sh: maturin succeeded but 'import openjarvis_rust'"
|
||||
echo "failed in the serving venv ($SRC_DIR/.venv) — the extension was"
|
||||
echo "not installed where the server runs. (exit=$rc)"
|
||||
tail -n 50 "$LOG" 2>/dev/null || true
|
||||
} > "$FAILED"
|
||||
rm -f "$BUILT"
|
||||
exit "$rc"
|
||||
fi
|
||||
tmp="$BUILT.tmp"
|
||||
date -u +"%Y-%m-%dT%H:%M:%SZ" > "$tmp"
|
||||
mv "$tmp" "$BUILT"
|
||||
|
||||
@@ -153,14 +153,30 @@ memory_router = APIRouter(prefix="/v1/memory", tags=["memory"])
|
||||
|
||||
|
||||
def _get_memory_backend(request: Request):
|
||||
"""Return the app-level memory backend, falling back to a fresh SQLiteMemory."""
|
||||
"""Return the app-level memory backend, falling back to a fresh SQLiteMemory.
|
||||
|
||||
Raises ``HTTPException(503)`` with an actionable message when the backend
|
||||
cannot be built because the mandatory ``openjarvis_rust`` extension is not
|
||||
installed in the serving venv. This is deliberately distinct from a benign
|
||||
"memory not configured" case (which returns ``None``): a missing native
|
||||
extension must fail loudly, never silently degrade (#502).
|
||||
"""
|
||||
backend = getattr(request.app.state, "memory_backend", None)
|
||||
if backend is None:
|
||||
from openjarvis.tools.storage._stubs import MemoryBackendUnavailable
|
||||
|
||||
try:
|
||||
from openjarvis.tools.storage.sqlite import SQLiteMemory
|
||||
|
||||
backend = SQLiteMemory()
|
||||
except MemoryBackendUnavailable as exc:
|
||||
# The native extension is missing — surface a loud, actionable error
|
||||
# rather than a misleading "no backend" / silent no-op.
|
||||
logger.error("%s", exc)
|
||||
raise HTTPException(status_code=503, detail=str(exc)) from exc
|
||||
except Exception:
|
||||
# Memory is genuinely unconfigured for a benign reason — preserve
|
||||
# the existing graceful "no backend" behaviour.
|
||||
return None
|
||||
return backend
|
||||
|
||||
@@ -170,7 +186,9 @@ async def memory_store(req: MemoryStoreRequest, request: Request):
|
||||
"""Store content in memory."""
|
||||
backend = _get_memory_backend(request)
|
||||
if backend is None:
|
||||
return {"status": "stored", "note": "no backend available"}
|
||||
# Memory is intentionally disabled; report it honestly instead of a
|
||||
# 200 that silently discards the write (#502).
|
||||
raise HTTPException(status_code=503, detail="Memory is not configured")
|
||||
try:
|
||||
backend.store(req.content, metadata=req.metadata or {})
|
||||
return {"status": "stored"}
|
||||
@@ -216,7 +234,13 @@ async def memory_stats(request: Request):
|
||||
|
||||
@memory_router.get("/config")
|
||||
async def memory_config(request: Request):
|
||||
"""Return current memory configuration."""
|
||||
"""Return current memory configuration.
|
||||
|
||||
Reports memory as *unavailable* (rather than falsely claiming
|
||||
``backend_type: sqlite``) when the native ``openjarvis_rust`` extension is
|
||||
missing, so the UI can show the real cause instead of a healthy-looking
|
||||
config that backs a silent no-op (#502).
|
||||
"""
|
||||
try:
|
||||
config = getattr(request.app.state, "config", None)
|
||||
if config is None:
|
||||
@@ -224,12 +248,30 @@ async def memory_config(request: Request):
|
||||
|
||||
config = load_config()
|
||||
backend = getattr(request.app.state, "memory_backend", None)
|
||||
available = True
|
||||
detail: Optional[str] = None
|
||||
if backend is None:
|
||||
from openjarvis.tools.storage._stubs import MemoryBackendUnavailable
|
||||
|
||||
try:
|
||||
from openjarvis.tools.storage.sqlite import SQLiteMemory
|
||||
|
||||
backend = SQLiteMemory()
|
||||
except MemoryBackendUnavailable as exc:
|
||||
available = False
|
||||
detail = str(exc)
|
||||
except Exception:
|
||||
# Benign: cannot construct a probe backend here, but the
|
||||
# configured default is still what would be used.
|
||||
pass
|
||||
return {
|
||||
"backend_type": (
|
||||
backend.backend_id
|
||||
if backend is not None
|
||||
else config.memory.default_backend
|
||||
),
|
||||
"available": available,
|
||||
"detail": detail,
|
||||
"context_top_k": config.memory.context_top_k,
|
||||
"context_min_score": config.memory.context_min_score,
|
||||
"context_max_tokens": config.memory.context_max_tokens,
|
||||
@@ -253,7 +295,7 @@ async def memory_index(req: MemoryIndexRequest, request: Request):
|
||||
|
||||
backend = _get_memory_backend(request)
|
||||
if backend is None:
|
||||
raise HTTPException(status_code=503, detail="No memory backend available")
|
||||
raise HTTPException(status_code=503, detail="Memory is not configured")
|
||||
|
||||
chunks = ingest_path(target)
|
||||
stored = 0
|
||||
@@ -264,7 +306,16 @@ async def memory_index(req: MemoryIndexRequest, request: Request):
|
||||
backend.store(chunk.content, metadata=metadata)
|
||||
stored += 1
|
||||
|
||||
return {"status": "indexed", "chunks_indexed": stored}
|
||||
result = {"status": "indexed", "chunks_indexed": stored}
|
||||
if stored == 0:
|
||||
# "indexed" must never silently mean "stored nothing". Surface why
|
||||
# so a folder of short notes doesn't look like a successful no-op
|
||||
# (#502 follow-up).
|
||||
result["note"] = (
|
||||
"no content was indexed — the path contained no readable "
|
||||
"documents with indexable text"
|
||||
)
|
||||
return result
|
||||
except HTTPException:
|
||||
raise
|
||||
except Exception as exc:
|
||||
|
||||
@@ -10,6 +10,32 @@ from abc import ABC, abstractmethod
|
||||
from dataclasses import dataclass, field
|
||||
from typing import Any, Dict, List, Optional
|
||||
|
||||
#: Actionable message surfaced whenever a Rust-backed memory backend cannot be
|
||||
#: constructed because the mandatory ``openjarvis_rust`` extension is missing
|
||||
#: from the *current* venv. Kept as a single constant so the server routes, the
|
||||
#: SDK and the regression tests all surface exactly the same wording.
|
||||
RUST_MISSING_HINT = (
|
||||
"Memory backend unavailable: the native `openjarvis_rust` extension is not "
|
||||
"installed in this environment. Build it into the venv that runs the server "
|
||||
"with `uv run maturin develop -m rust/crates/openjarvis-python/Cargo.toml` "
|
||||
"(needs rustc >= 1.88), then restart. Verify with "
|
||||
'`python -c "from openjarvis._rust_bridge import RUST_AVAILABLE; '
|
||||
'print(RUST_AVAILABLE)"`.'
|
||||
)
|
||||
|
||||
|
||||
class MemoryBackendUnavailable(RuntimeError):
|
||||
"""Raised when a memory backend cannot be built because the mandatory
|
||||
``openjarvis_rust`` extension is missing from the current environment.
|
||||
|
||||
This is deliberately distinct from "memory is intentionally disabled": a
|
||||
missing native extension is an environment/install error that must be
|
||||
surfaced loudly and actionably, never swallowed into a silent no-op.
|
||||
"""
|
||||
|
||||
def __init__(self, message: str = RUST_MISSING_HINT) -> None:
|
||||
super().__init__(message)
|
||||
|
||||
|
||||
@dataclass(slots=True)
|
||||
class RetrievalResult:
|
||||
@@ -59,4 +85,9 @@ class MemoryBackend(ABC):
|
||||
"""Remove all stored documents."""
|
||||
|
||||
|
||||
__all__ = ["MemoryBackend", "RetrievalResult"]
|
||||
__all__ = [
|
||||
"RUST_MISSING_HINT",
|
||||
"MemoryBackend",
|
||||
"MemoryBackendUnavailable",
|
||||
"RetrievalResult",
|
||||
]
|
||||
|
||||
@@ -135,10 +135,17 @@ def chunk_text(
|
||||
current_tokens.extend(para_tokens)
|
||||
current_offset += len(para_tokens)
|
||||
|
||||
# Flush remaining tokens
|
||||
# Flush remaining tokens.
|
||||
#
|
||||
# ``min_chunk_size`` exists to discard tiny *trailing* fragments once a
|
||||
# document has already produced at least one chunk. It must NOT silently
|
||||
# drop an entire short document: indexing a folder of short notes would
|
||||
# otherwise report success while storing nothing (#502 follow-up). So if no
|
||||
# chunk has been emitted yet, keep the remaining content regardless of the
|
||||
# floor.
|
||||
if current_tokens:
|
||||
chunk_content = " ".join(current_tokens)
|
||||
if _count_tokens(chunk_content) >= cfg.min_chunk_size:
|
||||
if not chunks or _count_tokens(chunk_content) >= cfg.min_chunk_size:
|
||||
chunks.append(
|
||||
Chunk(
|
||||
content=chunk_content,
|
||||
|
||||
@@ -9,7 +9,11 @@ from typing import Any, Dict, List, Optional
|
||||
|
||||
from openjarvis.core.events import EventType, get_event_bus
|
||||
from openjarvis.core.registry import MemoryRegistry
|
||||
from openjarvis.tools.storage._stubs import MemoryBackend, RetrievalResult
|
||||
from openjarvis.tools.storage._stubs import (
|
||||
MemoryBackend,
|
||||
MemoryBackendUnavailable,
|
||||
RetrievalResult,
|
||||
)
|
||||
|
||||
|
||||
def _check_fts5(conn: sqlite3.Connection) -> bool:
|
||||
@@ -40,7 +44,15 @@ class SQLiteMemory(MemoryBackend):
|
||||
|
||||
from openjarvis._rust_bridge import get_rust_module
|
||||
|
||||
_rust = get_rust_module()
|
||||
# The Rust backend is mandatory and there is no Python fallback. When
|
||||
# the extension is missing from *this* venv, ``get_rust_module`` raises
|
||||
# ImportError; translate it into a clear, actionable error so callers
|
||||
# never degrade to a misleading "Failed to index path" or a silent
|
||||
# no-op (see #502).
|
||||
try:
|
||||
_rust = get_rust_module()
|
||||
except ImportError as exc:
|
||||
raise MemoryBackendUnavailable() from exc
|
||||
self._rust_impl = _rust.SQLiteMemory(self._db_path)
|
||||
self._conn = None # type: ignore[assignment]
|
||||
|
||||
|
||||
@@ -66,13 +66,40 @@ def test_custom_config():
|
||||
assert len(chunks) >= 3
|
||||
|
||||
|
||||
def test_min_chunk_size_filters_tiny():
|
||||
def test_short_only_document_not_dropped():
|
||||
"""A whole document below min_chunk_size must still produce a chunk.
|
||||
|
||||
Regression for #502 follow-up: previously a folder of short notes indexed
|
||||
to ``chunks_indexed: 0`` (HTTP 200), silently storing nothing. ``min_chunk_size``
|
||||
should only discard tiny *trailing fragments*, never an entire short doc.
|
||||
"""
|
||||
cfg = ChunkConfig(chunk_size=100, chunk_overlap=0, min_chunk_size=50)
|
||||
# 30 words is below min_chunk_size=50
|
||||
# 30 words is below min_chunk_size=50, but it's the entire document.
|
||||
words = [f"w{i}" for i in range(30)]
|
||||
text = " ".join(words)
|
||||
chunks = chunk_text(text, config=cfg)
|
||||
assert len(chunks) == 0
|
||||
assert len(chunks) == 1
|
||||
assert chunks[0].content == text
|
||||
|
||||
|
||||
def test_short_real_world_note_not_dropped():
|
||||
"""The exact repro from the issue: a ~4-word note must not vanish."""
|
||||
chunks = chunk_text("hello world\nsome content\n", source="a.txt")
|
||||
assert len(chunks) == 1
|
||||
assert "hello world" in chunks[0].content
|
||||
|
||||
|
||||
def test_min_chunk_size_filters_tiny_trailing_fragment():
|
||||
"""A tiny fragment trailing a real chunk is still dropped by the floor."""
|
||||
cfg = ChunkConfig(chunk_size=50, chunk_overlap=0, min_chunk_size=10)
|
||||
# Two paragraphs: the first fills a real chunk, the second is a tiny tail.
|
||||
para1 = " ".join(f"a{i}" for i in range(50))
|
||||
para2 = " ".join(f"b{i}" for i in range(3)) # 3 words < min_chunk_size=10
|
||||
text = f"{para1}\n\n{para2}"
|
||||
chunks = chunk_text(text, config=cfg)
|
||||
# The 3-word trailing fragment is discarded; only the real chunk remains.
|
||||
assert len(chunks) == 1
|
||||
assert "b0" not in chunks[0].content
|
||||
|
||||
|
||||
def test_source_propagated():
|
||||
|
||||
@@ -49,6 +49,54 @@ class TestMemoryRoutes:
|
||||
assert resp.status_code in (200, 500)
|
||||
|
||||
|
||||
class TestMemoryRustMissing:
|
||||
"""Regression for #502: when the native ``openjarvis_rust`` extension is
|
||||
missing from the serving venv, memory ops must surface a CLEAR, ACTIONABLE
|
||||
error — never the misleading "Failed to index path" or a 200 silent no-op.
|
||||
"""
|
||||
|
||||
@staticmethod
|
||||
def _client(monkeypatch):
|
||||
# Force the same failure mode as a venv without the compiled extension.
|
||||
def _boom():
|
||||
raise ImportError("No module named 'openjarvis_rust'")
|
||||
|
||||
import openjarvis._rust_bridge as bridge
|
||||
|
||||
monkeypatch.setattr(bridge, "get_rust_module", _boom)
|
||||
return TestClient(_make_app())
|
||||
|
||||
def test_store_is_not_a_silent_noop(self, monkeypatch):
|
||||
client = self._client(monkeypatch)
|
||||
resp = client.post("/v1/memory/store", json={"content": "hi"})
|
||||
# Must NOT return the old 200 {"status":"stored","note":"no backend..."}.
|
||||
assert resp.status_code == 503
|
||||
detail = resp.json()["detail"]
|
||||
assert "openjarvis_rust" in detail
|
||||
assert "maturin develop" in detail
|
||||
|
||||
def test_index_surfaces_actionable_detail(self, monkeypatch, tmp_path):
|
||||
(tmp_path / "note.txt").write_text("hello world some content here")
|
||||
client = self._client(monkeypatch)
|
||||
resp = client.post("/v1/memory/index", json={"path": str(tmp_path)})
|
||||
assert resp.status_code == 503
|
||||
detail = resp.json()["detail"]
|
||||
# The frontend reads this `detail`; it must point at the real cause,
|
||||
# not blame the indexed path.
|
||||
assert "openjarvis_rust" in detail
|
||||
assert detail != "Failed to index path"
|
||||
assert detail != "No memory backend available"
|
||||
|
||||
def test_config_reports_unavailable(self, monkeypatch):
|
||||
client = self._client(monkeypatch)
|
||||
resp = client.get("/v1/memory/config")
|
||||
assert resp.status_code == 200
|
||||
data = resp.json()
|
||||
# Must not falsely report a healthy backend when none could be built.
|
||||
assert data["available"] is False
|
||||
assert "openjarvis_rust" in (data["detail"] or "")
|
||||
|
||||
|
||||
class TestBudgetRoutes:
|
||||
def test_get_budget(self):
|
||||
client = TestClient(_make_app())
|
||||
|
||||
Reference in New Issue
Block a user