Conversation
…sources per merge Reflection deprecated the sources of a merge with one LanceDB update per episode and per fact parent, all started concurrently. Every update takes the table write lock, so on a large cluster (228 sources in the report) the calls queued behind each other until they hit the 15 s write-lock deadline; the run was then reported failed while the updates that had already gone through stayed applied, leaving sources half-deprecated toward a merge that was never committed to the cluster. - Deprecate in batches of 100 ids with one `entry_id IN (...)` / `parent_id IN (...)` update each, issued sequentially, so a merge takes a handful of lock acquisitions instead of hundreds. - Write the LanceDB rows first and the markdown frontmatter last, and on any failure clear the `deprecated_by` values this run wrote (only rows pointing at this merge's entry) before propagating the error. A failed run now leaves the sources, the markdown record and the cluster as they were. - Cap one merge at 50 source episodes. A larger cluster merges its existing merged episode plus the oldest sources; the deferred members stay in the cluster (its count now reflects them) and are folded in by later runs in update mode. Refs EverMind-AI#443 (the prompt/language configuration is left for a separate change)
|
The LanceDB-first / markdown-last ordering is a good fix for the half-deprecated failure mode. I like that the compensation predicate clears only rows pointing at this merge entry id; that avoids undoing unrelated deprecations from another successful run. One edge I would want covered, either by test or review note: retry after a compensated failure should be idempotent when the failed run created the merged episode row but did not patch markdown frontmatter. In other words, the orphan detection path should either remove/ignore that merge candidate before the retry or guarantee it cannot be selected as a source and produce a duplicate reflection. The source cap also changes operator expectations a bit: a large cluster is now intentionally convergent across multiple runs, not atomic in one run. If there is run telemetry, exposing |
2 participants
Summary
Addresses the first two defects in #443 (Reflection V1 on large clusters):
Why the run failed.
_deprecate_lance_episodes/_deprecate_lance_factsissued one LanceDBupdateper source episode and per fact parent, all started concurrently withasyncio.gather. Everyupdatetakes the table's write lock, so on a 228-member cluster the calls queued behind one another until the 15 s write-lock deadline (lancedb_write_lock_deadline_exceededstorm), the run was reported failed, and the updates that had already gone through stayed applied: sources half-deprecated toward a merge that was never committed to the cluster.Changes
entry_id IN (...)(episodes) /parent_id IN (...)(facts) update each, issued sequentially. A merge now takes a handful of lock acquisitions instead of hundreds.deprecated_byvalues written by this run (only rows pointing at this merge's entry id) are cleared before the error propagates. A failed run now leaves the sources, the markdown record and the cluster as they were; the merged episode itself is still detected by the existing orphan check on the next run, as before._MAX_SOURCES_PER_MERGE = 50(module constant next to_MAX_CLUSTERS_PER_RUN). A larger cluster merges its existing merged episode(s) plus the oldest sources up to the cap; deferred members stay in the cluster, are neither merged nor deprecated in this run, and are folded in by later runs in update mode. The cluster'scountafter a merge now reflects the members that remain (previously hard-coded to 1). Wiring the cap into[reflection]config can follow if you prefer it configurable.The third point in the issue (prompt / output-language configuration) is a product decision and is left out.
Area
Verification
New tests in
tests/unit/test_memory/test_reflection/test_orchestrator.py:test_deprecate_lance_episodes_batches_updates: 250 ids → 3 updates withINpredicates covering every id.test_deprecate_failure_reverts_applied_writes: the second batch raisesVectorStoreBusyError→ the markdown record is never patched, both applied batches are reverted (deprecated_by = NULLonly where it equals this merge's entry id), and the error propagates so the run is still reported failed.test_run_caps_sources_per_merge_and_keeps_the_rest: with the cap at 2 and a 3-member cluster, only the two oldest sources are reflected, deprecated and removed; the deferred member plus the merged episode remain (count=2).Checklist
main..envfiles, dependency folders, or generated output.Notes for Reviewers
The reorder (LanceDB first, markdown last) is what makes the compensation complete without having to un-patch frontmatter. If you would rather keep markdown first, the revert still works for the LanceDB side, but the frontmatter would then re-apply the deprecation on the next cascade reconcile.
Refs #443