Systematic code review — batch of fixes incoming (tracking)

[KeilerHirsch]

Context

I've been running a systematic code review of mempalace as a heavy daily user of the CLI + MCP server. It ran over several days across roughly the whole codebase (~46k LOC) — module by module, plus dedicated cross-cutting passes for concurrency, silent failures, data loss, Windows behavior, and encoding. That depth is why the batch of fixes ends up being this large, and I wanted to give you the map up front rather than let the PR count climb without context.

To be transparent about how the findings were produced (so you can weigh them accordingly): the review was run as a multi-agent pass, and every finding was put through an independent adversarial verification step — three separate checks per finding (does the mechanism actually hold / is it reachable in real use / does prior art confirm it), kept only on majority-not-refuted. That filter mattered: it threw out roughly a quarter of the raw candidates (164 flagged → 126 confirmed), so what's left is the set that survived scrutiny, not a raw dump. The confirmed findings break down as 7 critical / 57 high / 38 medium / 24 low, organized into 64 PR-sized groups — which is the queue these PRs are working through.

None of that is a substitute for your review, of course — it's just why there are this many, and why I've tried to make each one cheap to check:

• a failing regression test first (red), then the fix (green), verified by reverting only the source files;
• the existing suite kept green (no regressions);
• ruff check and ruff format --check clean;
• an independent review pass before pushing.

Each PR is scoped to one defect class in one subsystem, so they can be reviewed and merged independently, in whatever order suits you.

PRs open so far

Repair / index integrity

• fix(repair): preserve verified temp copy on failed rebuild swap; fail loud on truncated pagination #2086 — preserve the verified temp copy on a failed rebuild swap; fail loud on truncated pagination
• fix(repair): stop dropping drawers with zero embedding_metadata rows #2087 — stop dropping drawers with zero embedding_metadata rows (LEFT JOIN)
• fix: preflight HNSW divergence before every col.count() call site the audit found #2093 — run the existing HNSW-divergence preflight at every col.count() call site, not just the four it currently guards

Miner / mining safety

• fix(miner): close four re-mine safety gaps in process_file #2088 — close four re-mine safety gaps in process_file (completion marker, TOCTOU mtime, swallowed purge error, unconditional closet purge)
• fix(convo_miner): stop sweeper drawers being purged, abort on failed purge #2089 — stop convo_miner's sweeper drawers being purged on re-mine; abort on a failed purge

Cross-platform / correctness

• fix: replace fcntl.flock spawn-mutex with a cross-platform primitive (Windows no-op today) #2096 — replace the fcntl.flock spawn-mutex with a cross-platform primitive (it's a silent no-op on Windows today)
• fix(encoding): pin encoding=utf-8 on dialect.py + config.py text opens #2098 — pin encoding="utf-8" on the text-mode open() calls that relied on the OS locale codepage (umlaut mojibake on non-UTF-8 Windows)
• fix(dialect): whole-word entity match + wire lang_regex into extraction (#50, #53) #2099 — whole-word entity match in encode_entity (a short registered name was claiming a longer one's code) + actually consult the per-locale lang_regex in extraction, which was loaded but never used

All of the above are CI-green and have no merge conflicts.

What's still coming

The review surfaced a number of additional issues across repair.py, migrate.py, backups.py, the backend adapters, and the i18n/extraction paths. I'll keep sending them in small, single-purpose PRs and checking them off here. A few categories I'll flag rather than silently PR:

• Design proposals (not bug fixes) — e.g. a retroactive collision scan — I'll raise as separate discussion issues for your buy-in first, not as PRs.
• Backend-specific fixes (pgvector / qdrant / milvus) — I run the chromadb backend exclusively, so I can't personally verify those against a live instance; I'll validate them against each backend's own test doubles and say so explicitly in the PR, rather than claim live verification I don't have.

The one that matters most

The recurring HNSW-vs-SQLite divergence traces back to the rebuild path not holding mine_palace_lock() for the duration of the rebuild, so writes that land mid-rebuild are lost from the index. #2086 + #2093 harden the blast radius, but the root cause — keeping writes and the rebuild from racing — is a larger change (a write-queue / quiescence design) that I'd rather scope with you before implementing, since it touches the core write path. Happy to open a design issue for that specifically if you'd like.

Not asking for anything urgent here — just wanted to give you the map before the PR count climbs. Thanks for the project; it's become a core part of how I work.

[fatkobra]

Thanks for the transparency around the audit methodology and for grouping
the findings into reviewable changes rather than sending one large patch.
The red-before/green-after approach and the explicit disclosure of backend
testing limitations are especially helpful.

One coordination point on “The one that matters most”:

The rebuild snapshot race looks like a valid and important writer path to
investigate, but I do not think it should yet be described as the root cause
of the complete recurring-divergence cluster.

#1963 already tracks the broader architectural problem: multiple MCP, hook,
CLI, daemon, and maintenance processes can independently become local
backend writers. #1976 and #2079 are also active implementation work around
daemon routing and process-lifetime ownership.

The repair rebuild appears to be another path that must participate in that
same ownership model. Before implementing a separate write-queue or
quiescence design, could the proposed design issue explicitly map:

• how it relates to Epic: concurrent-writer HNSW corruption — unified tracking for the recurring-divergence cluster (357, 1092, 1581, 1799, 1845, 1888, chroma#6975, …) #1963;
• whether it extends the existing daemon/lease model or introduces another
  coordination mechanism;
• how direct maintenance operations such as repair and regeneration enter
  quiescence;
• whether readers remain allowed during the rebuild;
• what happens to writes already queued when promotion or rollback fails;
• and an interleaving regression that proves a write cannot disappear
  between extraction and live-collection replacement?

Given the planned 64 PR-sized groups, it may also help to let maintainers
prioritize a small first tranche before the full queue is opened. That
would reduce overlap and allow later findings to be rebased against the
behaviour established by the earlier fixes.

The tracking map is useful; I would just keep the severity and
“confirmed” status provisional until each finding has been independently
reviewed against the real execution path.

@mvalentsev any thoughts?

cc @igorls

[mvalentsev]

Hey @fatkobra, on your second question: extend the existing model. The gap isn't that repair skips the palace lock, it's that it takes it per write instead of for the whole operation. In three runs of rebuild_index on current develop a second process took mine_palace_lock in the middle of the rebuild and the rebuild still finished; in three more runs where that process wrote a drawer, the rebuild aborted with "palace is held by PID ...". So what I actually hit is the rebuild giving up, not a write disappearing. Either way that's the "separate exclusive-maintenance policy" #2079's doc reserves and leaves empty. @KeilerHirsch's #2031 already runs reconcile under one lease, and #1598 does the same for rebuild_from_sqlite once it's rebased.

On "confirmed" staying provisional: you flagged the closet phase on #2088, and there's a second one pointing the other way. Dedup deletes drawers inside a source_file group, which invalidates the chunk_total marker for good, so the next mine refiles the whole file and restores what dedup removed. Repro goes on the PR.

[fatkobra]

Thanks @mvalentsev — understood.

So the reproduced failure is that a competing writer can cause the rebuild
to abort, rather than a write being silently lost. The appropriate direction
is still to extend the existing model by holding one palace lease for the
complete maintenance operation, as #2031 and #1598 already do, rather than
introducing a separate coordination mechanism.

I will keep the broader root-cause wording provisional and follow the
chunk_total/dedup reproduction on #2088 rather than duplicate it here.