Skip to content

Commit c4ef4f0

Browse files
committed
fix(status): wire CLI status() to get_all_metadata() fast path (#2153)
CLI `mempalace status` (`miner.py:status()`) still used an O(n²) offset-paginated loop even after the MCP server path was fixed in #1796/#1836. For Qdrant, each `col.get(offset=N)` call materializes the entire collection via `_scroll_all()` then slices — so ~32 full re-fetches for a 159k-drawer palace. Fix: after the chroma sqlite fast path returns None and the collection is opened, check `getattr(col, "get_all_metadata", None)`. If callable, fetch all metadata in one pass, tally wing/room counts, and return. The existing offset loop remains as fallback for backends that don't expose the contract method. This mirrors the delegation pattern already used by `mcp_server._fetch_all_metadata()` (#1796). Before: 1m46s on 159,601-drawer Qdrant palace After: 3.4s (31x speedup) Test: `tests/test_cli_status_fast_path.py` asserts status() uses get_all_metadata() when available and does NOT call the offset loop, plus fallback coverage for legacy collections.
1 parent 1d113e4 commit c4ef4f0

2 files changed

Lines changed: 256 additions & 0 deletions

File tree

mempalace/miner.py

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2161,6 +2161,18 @@ def status(palace_path: str):
21612161
print(" Run `mempalace repair --mode from-sqlite --archive-existing` first.")
21622162
return
21632163

2164+
# Fast path: single-pass metadata fetch for backends that expose
2165+
# get_all_metadata() (#1796). Avoids the O(n^2) offset loop below.
2166+
get_all = getattr(col, "get_all_metadata", None)
2167+
if callable(get_all):
2168+
metas = get_all()
2169+
wing_rooms: dict = defaultdict(lambda: defaultdict(int))
2170+
for m in metas:
2171+
m = m or {}
2172+
wing_rooms[m.get("wing", "?")][m.get("room", "?")] += 1
2173+
_print_status(len(metas), wing_rooms)
2174+
return
2175+
21642176
# Count by wing and room — paginate to avoid SQLite "too many SQL
21652177
# variables" error on large palaces (see #802, #850).
21662178
total = col.count()

tests/test_cli_status_fast_path.py

Lines changed: 244 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,244 @@
1+
"""
2+
Tests for issue #2153 -- CLI ``mempalace status`` O(n^2) offset loop on
3+
backends that expose ``get_all_metadata()``.
4+
5+
The CLI path (``miner.status()``) was never wired to the
6+
``get_all_metadata()`` contract method that the MCP server already uses
7+
(``mcp_server._fetch_all_metadata``, PR #1796). For backends like Qdrant
8+
whose ``get(limit=, offset=)`` is backed by a full ``_scroll_all()``
9+
materialization, the offset loop re-walks the entire collection on every
10+
page -- O(n^2) in collection size.
11+
12+
These tests verify the fix: ``miner.status()`` must use
13+
``get_all_metadata()`` when the collection exposes it, and must NOT fall
14+
through to the offset-paginated ``col.get()`` loop. The fallback path is
15+
also exercised for collections that predate the contract method.
16+
"""
17+
18+
from unittest import mock
19+
20+
import pytest
21+
22+
23+
# ---------------------------------------------------------------------------
24+
# Helpers
25+
# ---------------------------------------------------------------------------
26+
27+
28+
def _make_collection_with_get_all(metadata_list):
29+
"""A mock collection that exposes get_all_metadata() and tracks get() calls.
30+
31+
The returned object records every call to ``get()`` so tests can assert
32+
the offset loop was NOT entered.
33+
"""
34+
35+
class _Collection:
36+
def __init__(self):
37+
self._meta = metadata_list
38+
self.get_calls = []
39+
40+
def get_all_metadata(self, where=None):
41+
return list(self._meta)
42+
43+
def get(self, *, ids=None, where=None, limit=None, offset=None, include=None):
44+
self.get_calls.append({"limit": limit, "offset": offset})
45+
# Simulate what a Qdrant backend does: materialize everything, slice
46+
offset = offset or 0
47+
limit = limit if limit is not None else len(self._meta)
48+
page = self._meta[offset : offset + limit]
49+
return {"ids": [], "documents": [], "metadatas": page}
50+
51+
def count(self):
52+
return len(self._meta)
53+
54+
return _Collection()
55+
56+
57+
class _LegacyCollection:
58+
"""Collection WITHOUT get_all_metadata() -- triggers the fallback loop."""
59+
60+
def __init__(self, metadata_list):
61+
self._meta = metadata_list
62+
63+
def get(self, *, ids=None, where=None, limit=None, offset=None, include=None):
64+
offset = offset or 0
65+
limit = limit if limit is not None else len(self._meta)
66+
page = self._meta[offset : offset + limit]
67+
return {"ids": [], "documents": [], "metadatas": page}
68+
69+
def count(self):
70+
return len(self._meta)
71+
72+
73+
@pytest.fixture
74+
def _stub_status_dependencies(monkeypatch):
75+
"""Stub out the chroma-only and palace-opening deps so status() is unit-testable.
76+
77+
- ``_sqlite_wing_room_counts`` returns None (non-chroma backend)
78+
- ``hnsw_capacity_status`` returns a non-diverged dict
79+
- ``_open_collection_or_explain`` returns the mock collection
80+
"""
81+
monkeypatch.setattr(
82+
"mempalace.backends.chroma._sqlite_wing_room_counts",
83+
lambda palace_path, collection_name: None,
84+
)
85+
monkeypatch.setattr(
86+
"mempalace.backends.chroma.hnsw_capacity_status",
87+
lambda palace_path, collection_name: {"diverged": False, "status": "unknown"},
88+
)
89+
yield
90+
91+
92+
# ---------------------------------------------------------------------------
93+
# 1. Fast path: get_all_metadata() is used when present
94+
# ---------------------------------------------------------------------------
95+
96+
97+
class TestStatusUsesGetAllMetadata:
98+
"""miner.status() must delegate to get_all_metadata() when the collection
99+
exposes it, avoiding the O(n^2) offset loop."""
100+
101+
def test_uses_get_all_metadata_when_present(self, _stub_status_dependencies, monkeypatch, capsys):
102+
from mempalace import miner
103+
104+
meta = [
105+
{"wing": "sessions", "room": "technical"},
106+
{"wing": "sessions", "room": "technical"},
107+
{"wing": "sessions", "room": "planning"},
108+
{"wing": "knowledge", "room": "decisions"},
109+
]
110+
col = _make_collection_with_get_all(meta)
111+
monkeypatch.setattr(
112+
miner, "_open_collection_or_explain", lambda palace_path: col
113+
)
114+
115+
miner.status("/fake/palace")
116+
out = capsys.readouterr().out
117+
118+
# Correct total
119+
assert "4 drawers" in out
120+
# Correct wing/room breakdown
121+
assert "sessions" in out
122+
assert "technical" in out
123+
assert "planning" in out
124+
assert "knowledge" in out
125+
126+
def test_does_not_call_offset_loop_when_get_all_present(
127+
self, _stub_status_dependencies, monkeypatch, capsys
128+
):
129+
"""Regression guard: once get_all_metadata() is available, the offset
130+
loop must NOT execute -- doing both would silently double the read cost."""
131+
from mempalace import miner
132+
133+
col = _make_collection_with_get_all([{"wing": "a", "room": "b"}])
134+
monkeypatch.setattr(
135+
miner, "_open_collection_or_explain", lambda palace_path: col
136+
)
137+
138+
miner.status("/fake/palace")
139+
capsys.readouterr() # drain
140+
141+
assert col.get_calls == [], (
142+
f"get() offset loop must not run when get_all_metadata() is present, "
143+
f"but got {len(col.get_calls)} calls"
144+
)
145+
146+
def test_does_not_call_count_when_get_all_present(
147+
self, _stub_status_dependencies, monkeypatch, capsys
148+
):
149+
"""count() is only needed by the fallback offset loop; the fast path
150+
uses len(metas) instead."""
151+
from mempalace import miner
152+
153+
col = _make_collection_with_get_all([{"wing": "a", "room": "b"}])
154+
monkeypatch.setattr(
155+
miner, "_open_collection_or_explain", lambda palace_path: col
156+
)
157+
# Spy on count() -- it should NOT be called
158+
col.count = mock.MagicMock(side_effect=AssertionError("count() should not be called"))
159+
160+
miner.status("/fake/palace")
161+
capsys.readouterr()
162+
163+
col.count.assert_not_called()
164+
165+
def test_handles_none_metadata_cells(self, _stub_status_dependencies, monkeypatch, capsys):
166+
"""Partially-flushed rows can have None metadata; status() must coerce
167+
them to {} before calling .get() (same pattern as mcp_server)."""
168+
from mempalace import miner
169+
170+
meta = [
171+
{"wing": "sessions", "room": "technical"},
172+
None, # partially-flushed row
173+
{"wing": "sessions", "room": "planning"},
174+
]
175+
col = _make_collection_with_get_all(meta)
176+
monkeypatch.setattr(
177+
miner, "_open_collection_or_explain", lambda palace_path: col
178+
)
179+
180+
miner.status("/fake/palace")
181+
out = capsys.readouterr().out
182+
183+
assert "3 drawers" in out
184+
assert "?" in out # None metadata → wing "?" room "?"
185+
186+
def test_large_metadata_set_uses_single_pass(
187+
self, _stub_status_dependencies, monkeypatch, capsys
188+
):
189+
"""With 10k+ drawers, get_all_metadata() must still make only ONE call
190+
and NOT degrade to the offset loop."""
191+
from mempalace import miner
192+
193+
meta = [{"wing": "sessions", "room": "technical"} for _ in range(10000)]
194+
col = _make_collection_with_get_all(meta)
195+
monkeypatch.setattr(
196+
miner, "_open_collection_or_explain", lambda palace_path: col
197+
)
198+
199+
miner.status("/fake/palace")
200+
capsys.readouterr()
201+
202+
assert col.get_calls == [], "offset loop must not run for large collections"
203+
204+
205+
# ---------------------------------------------------------------------------
206+
# 2. Fallback: offset loop still works for legacy collections
207+
# ---------------------------------------------------------------------------
208+
209+
210+
class TestStatusFallbackOffsetLoop:
211+
"""When get_all_metadata() is NOT present, status() must fall back to the
212+
offset-paginated col.get() loop with byte-for-byte the same behavior
213+
as before the fix."""
214+
215+
def test_fallback_uses_offset_loop(self, _stub_status_dependencies, monkeypatch, capsys):
216+
from mempalace import miner
217+
218+
meta = [{"wing": "a", "room": "x"}, {"wing": "b", "room": "y"}]
219+
col = _LegacyCollection(meta)
220+
assert not hasattr(col, "get_all_metadata")
221+
222+
monkeypatch.setattr(
223+
miner, "_open_collection_or_explain", lambda palace_path: col
224+
)
225+
226+
miner.status("/fake/palace")
227+
out = capsys.readouterr().out
228+
229+
assert "2 drawers" in out
230+
assert "a" in out
231+
assert "b" in out
232+
233+
def test_fallback_empty_collection(self, _stub_status_dependencies, monkeypatch, capsys):
234+
from mempalace import miner
235+
236+
col = _LegacyCollection([])
237+
monkeypatch.setattr(
238+
miner, "_open_collection_or_explain", lambda palace_path: col
239+
)
240+
241+
miner.status("/fake/palace")
242+
out = capsys.readouterr().out
243+
244+
assert "0 drawers" in out

0 commit comments

Comments
 (0)