PR Review: bench: add scale benchmark suite (106 tests)

Executive Summary

Affected Areas: tests/benchmarks/ (new), pyproject.toml, .github/workflows/ci.yml

Business Impact: Enables data-driven performance decisions. Key findings already surfaced (recall ceiling at ~1K drawers/bucket, client re-instantiation overhead, unbounded metadata fetch).

Flow Changes: No production flow changes. Adds pytest markers, CLI options, and a pythonpath directive that affect all test invocations.

Ratings

PR Health

• Has clear description
• References related issues/PRs (fix: enable SQLite WAL mode and add consistent LIMIT to KG timeline #136, fix: add limit=10000 safety cap to all unbounded ChromaDB .get() calls #137, fix: sanitize error responses and remove sys.exit from library code #139, fix: sanitize SESSION_ID in save hook to prevent path traversal #141, chore: tighten chromadb version range and add py.typed marker #142)
• Appropriate size (2,851 lines — large but cohesive)
• Has relevant tests (it IS the tests)

High Priority Issues

(Must fix before merge)

🐛 #1: record_metric() has a read-modify-write race on a shared temp file

Location: tests/benchmarks/report.py:16-31 | Confidence: ✅ HIGH

record_metric() reads a JSON file, updates it in-memory, and writes it back — all without file locking. Under parallel test execution (pytest-xdist / -n auto), multiple processes will race on this file, silently dropping metrics or corrupting the JSON.

Even without xdist, pytest's fixture teardown ordering is not guaranteed across sessions, so the pytest_terminal_summary hook reading this file could hit a partially-written state.

- def record_metric(category: str, metric: str, value):
-     """Append a metric to the session results file (JSON on disk)."""
-     results = {}
-     if os.path.exists(RESULTS_FILE):
-         try:
-             with open(RESULTS_FILE) as f:
-                 results = json.load(f)
-         except (json.JSONDecodeError, OSError):
-             results = {}
-     if category not in results:
-         results[category] = {}
-     results[category][metric] = value
-     with open(RESULTS_FILE, "w") as f:
-         json.dump(results, f, indent=2)
+ import fcntl
+
+ def record_metric(category: str, metric: str, value):
+     """Append a metric to the session results file (JSON on disk)."""
+     with open(RESULTS_FILE, "a+") as f:
+         fcntl.flock(f, fcntl.LOCK_EX)
+         try:
+             f.seek(0)
+             content = f.read()
+             results = json.loads(content) if content.strip() else {}
+             if category not in results:
+                 results[category] = {}
+             results[category][metric] = value
+             f.seek(0)
+             f.truncate()
+             json.dump(results, f, indent=2)
+         finally:
+             fcntl.flock(f, fcntl.LOCK_UN)

Note: fcntl is Unix-only. For cross-platform support, use filelock (PyPI) or document that benchmarks are single-process only.

🏗️ #2: CI benchmark job is documented but not implemented

Location: .github/workflows/ci.yml:21 + tests/benchmarks/README.md:168-175 | Confidence: ✅ HIGH

The README documents a CI benchmark job:

benchmark:
  runs-on: ubuntu-latest
  if: github.event_name == 'pull_request'
  # Runs: pytest tests/benchmarks/ -m "benchmark and not stress and not slow" --bench-scale=small

But the actual CI change only adds --ignore=tests/benchmarks to the existing test step. No benchmark job is added. The regression detection infrastructure (check_regression(), --bench-report) is unreachable from CI.

  # In .github/workflows/ci.yml, after the existing test job:
+
+  benchmark:
+    runs-on: ubuntu-latest
+    if: github.event_name == 'pull_request'
+    steps:
+      - uses: actions/checkout@v4
+      - uses: actions/setup-python@v5
+        with:
+          python-version: "3.11"
+      - run: pip install -e ".[dev]"
+      - run: python -m pytest tests/benchmarks/ -v -m "benchmark and not stress and not slow" --bench-scale=small

Either add the job or remove the claim from the README.

Medium Priority Issues

(Should fix, not blocking)

🔗 #3: _get_rss_mb() duplicated across 5 test files

Location: test_chromadb_stress.py:20, test_ingest_bench.py:20, test_layers_bench.py:16, test_mcp_bench.py:47, test_memory_profile.py:20 | Confidence: ✅ HIGH

The identical 10-line _get_rss_mb() function is copy-pasted in 5 files. Any future bug fix or platform handling change must be replicated 5 times.

  # In conftest.py, add:
+ def _get_rss_mb():
+     """Get current process RSS in MB. Cross-platform."""
+     try:
+         import psutil
+         return psutil.Process().memory_info().rss / (1024 * 1024)
+     except ImportError:
+         import resource, platform
+         usage = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss
+         if platform.system() == "Darwin":
+             return usage / (1024 * 1024)
+         return usage / 1024
+
+ @pytest.fixture
+ def get_rss_mb():
+     return _get_rss_mb

Then import from conftest in each test file, or use it as a fixture.

⚡ #4: pythonpath = ["."] added globally affects all test runs

Location: pyproject.toml:67 | Confidence: ⚠️ MED

Adding pythonpath = ["."] to [tool.pytest.ini_options] modifies sys.path for every pytest invocation — not just benchmarks. This is needed because benchmarks use from tests.benchmarks.data_generator import ..., but it also means any top-level directory could shadow installed packages.

Since the project uses editable install (pip install -e ".[dev]"), this is currently harmless. But consider scoping it:

- pythonpath = ["."]

And instead adjust benchmark imports to be relative, or add a conftest.py at the tests/ root that handles path setup.

🐛 #5: Percentile calculations on 5 samples are statistically meaningless

Location: test_chromadb_stress.py:102, test_search_bench.py:51 | Confidence: ✅ HIGH

With only 5 query samples, p95 = sorted(latencies)[int(5 * 0.95)] = sorted(latencies)[4] = the maximum value. The "p95" label is misleading — it's just the max. Similarly, p50 with 5 samples is just the median of 5.

  # In test_query_latency_at_size and test_search_latency_curve:
  # Either increase the sample count:
- queries = [
-     "authentication middleware optimization",
-     ...  # 5 queries
- ]
+ queries = [...] * 6  # 30 total for meaningful percentiles

  # Or rename the metrics honestly:
- record_metric("chromadb_query", f"p95_latency_ms_at_{n_drawers}", round(p95_ms, 1))
+ record_metric("chromadb_query", f"max_latency_ms_at_{n_drawers}", round(max(latencies), 1))

🚨 #6: _patch_mcp_config silently ignores collection_name

Location: test_mcp_bench.py:32-45 (specifically line 39) | Confidence: ⚠️ MED

The monkeypatch replaces _file_config with {"palace_path": palace_path} but omits collection_name. This means _config.collection_name falls back to the default ("mempalace_drawers"), which happens to match the hardcoded collection name in populate_palace_directly(). If either default changes, these tests silently break.

- monkeypatch.setattr(cfg, "_file_config", {"palace_path": palace_path})
+ monkeypatch.setattr(cfg, "_file_config", {
+     "palace_path": palace_path,
+     "collection_name": "mempalace_drawers",
+ })

Low Priority Issues

(Nice to have)

🎨 #7: __import__("datetime") inline instead of normal import

Location: tests/benchmarks/conftest.py:118 | Confidence: ✅ HIGH

+ from datetime import datetime
  ...
  report = {
-     "timestamp": __import__("datetime").datetime.now().isoformat(),
+     "timestamp": datetime.now().isoformat(),

🏗️ #8: Double exclusion of benchmarks — marker filter AND --ignore

Location: pyproject.toml:68 + .github/workflows/ci.yml:21 | Confidence: ✅ HIGH

addopts = "-m 'not benchmark and not slow and not stress'" already excludes all benchmark tests from default pytest runs. The CI --ignore=tests/benchmarks is redundant. Having both is harmless but adds confusion about which mechanism is the source of truth.

The addopts marker approach is preferable — it also protects local pytest runs. The --ignore approach avoids even collecting the benchmark files (slightly faster). Pick one and document why.

🎨 #9: data_generator.py needle query extraction is fragile

Location: tests/benchmarks/data_generator.py:287-291 | Confidence: ⚠️ MED

The query extraction uses string splitting that depends on specific delimiters:

"query": topic.split(" uses ")[0]
    if " uses " in topic
    else topic.split(" set to ")[0]
    if " set to " in topic
    else topic[:60],

If a needle topic doesn't contain " uses " or " set to ", the query is just the first 60 chars, which may not be semantically distinct enough for recall measurement. Of 20 topics: 7 contain " uses ", 5 contain " set to ", and 8 fall through to the 60-char truncation path (40%). Consider adding explicit query strings per needle rather than relying on delimiter splitting.

Created by Octocode MCP https://octocode.ai