* fix(chroma): require SQLite magic header for ChromaBackend.detect() (#1893)
Closes#1893.
ChromaBackend.detect() was returning True for a 0-byte chroma.sqlite3 file
because the check was just os.path.isfile(...). On a palace that has any
other backend marker alongside a stale 0-byte chroma.sqlite3,
resolve_backend_name then raises BackendMismatchError and the palace becomes
unopenable until the user manually rm's the empty file.
The 0-byte file appears as a side effect of any sqlite3.connect() on a
missing path — Python creates the file immediately but writes the SQLite
header only on the first statement. So any code path that touches the
chroma.sqlite3 path with bare sqlite3.connect(), including chromadb's own
PersistentClient lazy-init (see the comment at backends/chroma.py:2052),
can leave a 0-byte artifact behind.
Fix: detect() now reads the first 16 bytes and compares to the SQLite
magic prefix b"SQLite format 3\x00" instead of relying on file presence
alone. One extra open() + 16-byte read; detect() isn't a hot path.
Properties:
- Rejects 0-byte files (the symptom #1893 is about).
- Rejects non-SQLite garbage at the canonical path (partial writes, etc.).
- Doesn't false-negative on real chroma palaces: any chroma palace whose
PersistentClient has done any work has the magic header on disk
(verified — CREATE TABLE is enough to land the header).
- Doesn't couple detect() to chroma's specific schema; the magic header
is stable across chromadb releases.
Test sweep: many test files used (chroma.sqlite3).touch() or
.write_bytes(b"") as a "fake palace" shortcut, exploiting the loose
isfile() check (one such site even had the comment "# pass the isfile
guard"). After this change, those stand-ins no longer register as chroma
palaces. Introduced tests/_chroma_palace_helper.py::make_minimal_chroma_sqlite
following the existing _backend_conformance.py precedent, and updated 15
call sites across 8 test files to use it. The existing
test_chroma_detect_matches_palace_with_chroma_sqlite (which encoded the
buggy semantics with write_bytes(b"")) is renamed to
test_chroma_detect_matches_palace_with_sqlite_header and now writes a
real SQLite database via the helper. Added two new tests for the
rejection paths (empty file, non-SQLite garbage).
Full env-cleared suite: 3137 passed, 20 skipped, 0 failed. ruff check
and ruff format --check both clean.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KC5Qsknh2zFRtRvVyjXiTA
* fix(sqlite_exact): require SQLite magic header for SQLiteExactBackend.detect()
Per gemini-code-assist review on #1892 PR #1896: SQLiteExactBackend has the
same os.path.isfile() detection pattern as ChromaBackend did, with the same
0-byte-file vulnerability. Mirrors the chroma fix for repo-wide consistency.
- SQLiteExactBackend.detect() now does the same 16-byte SQLite magic-prefix
check as ChromaBackend.detect().
- _chroma_palace_helper.py: factored its body into a private
_write_minimal_sqlite_file() and gained a sibling
make_minimal_sqlite_exact_sqlite() for the sqlite_exact filename. No churn
to any existing chroma call sites.
- test_sqlite_exact_backend.py:426 (the one site that wrote b"" for
sqlite_exact.sqlite3) updated to use the new helper.
- Three new tests in test_sqlite_exact_backend.py mirror the chroma trio:
matches with valid header, rejects empty file, rejects non-SQLite garbage.
Full env-cleared suite: 3140 passed, 20 skipped, 0 failed.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KC5Qsknh2zFRtRvVyjXiTA
---------
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Five fixes from the Copilot review of the merged daemon PR:
1. Privacy: the queue DB's SQLite WAL/SHM sidecars hold un-checkpointed
verbatim payloads but were created with the caller's umask. Set the
owner-only umask in run_server BEFORE DaemonRuntime builds the QueueStore
(not only once the HTTP server starts), and harden any existing sidecars in
QueueStore._init_db as defense-in-depth.
2. DoS guard: reject a negative Content-Length in the request reader.
rfile.read(-1) would block until the client disconnects and bypass the
MAX_BODY_BYTES cap.
3. Side effects: extract _wal_log (+ _ensure_wal, _WAL_FILE, _WAL_REDACT_KEYS)
into a new side-effect-free mempalace/wal.py. The CLI sync path and the
daemon service layer obtained _wal_log via `from .mcp_server import _wal_log`,
which runs mcp_server's import-time stdio protection (os.dup2(2, 1);
sys.stdout = sys.stderr) in a non-MCP process and misroutes operator output.
mcp_server/cli/service now import from mempalace.wal.
4. Correctness: run_mcp_tool treated any dict as success. Write tools that
return a bare {"error": ...} (e.g. tool_create_tunnel/tool_delete_tunnel
validation) were recorded as succeeded; now the "error" key infers failure.
5. Hook budget: get_client_if_running()/health() take an explicit timeout, and
the hook "is the daemon up?" precheck uses a short HOOK_PROBE_TIMEOUT (0.5s)
so a wedged daemon can't stall the hook for the default 5s.
Adds tests/test_wal.py (import isolation + redaction) and daemon tests for the
umask ordering, negative Content-Length, run_mcp_tool error inference, and the
short probe timeout.
CI was red on all three platforms for the daemon-mode draft PR. Root causes
and fixes:
- Linux 3.9 collection error: `_submit_daemon_job`'s `dedupe_key: str | None`
parameter annotation is evaluated at def time, and hooks_cli.py has no
`from __future__ import annotations` — `str | None` raises TypeError on 3.9.
Reverted to `dedupe_key: str = None` (the original, 3.9-safe). The other
`int | None` in the file is a function-local annotation, which is never
evaluated, so it was never the problem.
- macOS/Windows daemon lifecycle flakes: the 3 HTTP-lifecycle tests failed at
the 10s readiness deadline on contended CI runners (localhost bind is
sub-second locally but took ~5s when it passed on the macOS fleet, >10s when
it didn't), and because the server thread never shuts down on timeout,
run_server's `os.environ["MEMPALACE_PALACE_PATH"]` + `os.umask(0o077)`
mutations leaked into the rest of the suite — poisoning every later test that
reads MempalaceConfig().palace_path (the 60+ test_mcp_server cascade on macOS;
the at-exit socket hang → SIGINT on Windows). Bumped the readiness deadline
to 30s and added a module-scoped snapshot + autouse fixture in test_daemon.py
that force-restores the env + umask to the pre-suite baseline after every
daemon test, so a leaked server thread can't poison other test files.
Gemini review comments (fixed in code, no thread replies per convention):
- daemon.py `_connect()` was a bare `sqlite3.connect` whose `with`-block only
managed the transaction, not the connection — an unbounded FD leak in a
long-lived daemon running thousands of jobs (also the source of the Windows
"unclosed database" ResourceWarning noise). Converted to a closing
@contextlib.contextmanager.
- `QueueStore.finish()` gained `only_if_running`; `_safe_finish` passes it so a
late worker finish can't overwrite a shutdown-cancelled job back to
succeeded/failed — removes the reliance on process-exit timing.
- `DaemonClient.request` wraps the final `json.loads` in try/except
JSONDecodeError → DaemonError, so a non-JSON 2xx response surfaces as a
structured error instead of a bare JSONDecodeError.
- test_sync.py: removed the module-level `import mempalace.mcp_server` and moved
the stdout-rebinding side effect into an autouse fixture scoped to
TestServiceRunSyncReport, so the embedder/Chroma import chain is no longer
forced at collection time for the existing sync tests.
Coverage: added focused happy-path tests for service.run_sync early-returns,
run_mine backend application + invalid mode, execute_job kind dispatch,
run_diary_write arg forwarding, run_mcp_tool write-tool dispatch, and
print_job_result — lifts service.py from 57% to 85% so the new files
(service 85%, daemon 80%) don't drag the total below the 80% CI gate now that
the daemon tests complete and the gate is actually evaluated.
- New mempalace/daemon.py: long-lived localhost HTTP server (127.0.0.1) with a
SQLite WAL job queue, single worker thread, bearer-token auth, and owner-only
file perms (0600/0700) on queue DB, token, endpoint, and log.
- New mempalace/service.py: transport-neutral job execution surface shared by the
daemon, with per-job env isolation so one job's backend/palace switch cannot
leak into the next. mcp_tool is allowlisted to write-classified tools only.
- Crash recovery re-queues jobs left 'running' by a killed daemon; jobs that
already exhausted MAX_ATTEMPTS are dead-lettered to 'failed' instead of being
retried (non-idempotent diary_write would otherwise duplicate verbatim
content on every restart).
- Bounded retention prunes terminal jobs older than 7 days
(MEMPALACE_DAEMON_RETENTION_DAYS); queued/running jobs are never touched so a
crash mid-prune cannot drop in-flight work.
- CLI: --daemon/--background on mine/sync submit to the queue; new
`mempalace daemon {start,stop,status,jobs,wait}` subcommand. Strictly opt-in:
no flag, env, or config means no daemon and no behavior change.
- Hooks opt in via MEMPALACE_HOOKS_DAEMON or config hooks.daemon; when the daemon
is not already running, hooks fall back to the existing direct/spawn path so
the 500ms hook budget is preserved (hooks never auto-start the daemon).
- service.run_sync renders the same operator-facing report shape as the direct
CLI sync path (no_source, out_of_scope, by_source, Re-run/Removed hints) and
drops the old KeyError-prone 'deleted' read.
One-time mechanical reformat so `ruff format --check .` passes under the
newly pinned ruff. Layout only (assert-message parenthesization etc.),
no behavior change. 29 files: 28 under tests/ + 1 tools helper, no core
mempalace/ modules. Produced by `ruff format .`.
Windows runs treated `/tmp/elsewhere/x.md` as relative because Windows
absolute paths require a drive letter, so `_classify_drawer` routed
`drawer_out_of_scope` to `no_source` instead of `out_of_scope` and
`test_dry_run_classifies_correctly` failed on test-windows.
`Path(tmp_dir) / "elsewhere" / "x.md"` is absolute on every platform
and still lives outside the project root that the synced_world fixture
exposes via `repo_path`, so the bucket assertions hold cross-platform.
CI fix: `_classify_drawer` now resolves `source_file` symmetric to
`project_roots` (which `_normalize_project_dirs` and
`_auto_detect_project_roots` already `.resolve()`). Without this, on
platforms where the temp directory is a symlink (macOS `/var/folders` ->
`/private/var/folders`, Windows 8.3 short-name normalization), every
drawer mis-bucketed as `out_of_scope` and survived prune.
Perf:
- `_resolve_project_root`: early-return on first match (sorted-desc
precondition).
- `_normalize_project_dirs`: sort `(-len(str(p)), str(p))` desc for
early-return + deterministic tie-break on equal-length paths.
- `_auto_detect_project_roots`: `seen_sources` dedupe so a 200-chunk
file costs one disk walk, not 200.
- `sync_palace` main loop: per-file classification cache; registry
sentinels (`_reg_*`, `room=_registry`, `ingest_mode=registry`) routed
to "kept" before cache lookup so a sentinel sharing a `source_file`
with a pruned drawer cannot inherit a stale "gitignored" verdict.
- Closet purge: collapse O(N) per-file purge into one
`where={"source_file": {"$in": [...]}}` get + one bulk delete.
Tests (5 new in `TestSyncPalace`, 38 total):
- `test_symlinked_project_root_resolves`: pins symmetric resolve via
real `os.symlink` (skipped on Windows).
- `test_classification_cache_avoids_redundant_disk_hits`: monkeypatch
counter on `_classify_drawer` asserts `call_count == 1` for 5 chunks
sharing one source_file.
- `test_closet_batch_purge_single_call`: wraps closets collection with
`CallCountingCol` (forwards `.get`/`.delete`); asserts
`delete_calls == 1` and `get_calls == 1`; expected `removed_closets`
derived from `report["by_source"]` to stay robust to fixture changes.
- `test_registry_check_runs_before_cache_lookup`: a regular drawer
caches "gitignored" first; a sentinel with the same source_file must
still be kept.
- `test_normalize_project_dirs_sort_stable_on_equal_length`: pins the
alphabetical secondary key when paths share length.
Add `mempalace sync` CLI command and `mempalace_sync` MCP tool that
prune drawers whose source files are gitignored, deleted, or moved
out of the project. Reuses the existing GitignoreMatcher
infrastructure in mempalace/miner.py so the same gitignore rules
that block ingest also drive the corresponding cleanup.
Closes#1252.
Pim Messelink
Igor Lins e Silva
mvalentsev