Repository navigation
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 |
A run that fails after writing its merged episode (the deprecation writes fail and are reverted) leaves that episode live and outside the cluster. It is never loaded as a source, since sources come from cluster membership, but the retry wrote a second merged episode beside it, so the cluster ended with two live merges of the same sources. Orphan detection moves from the start of the run, where it only logged, to the deprecation step: live merged episodes of the cluster that are neither current members nor this run's merge are deprecated toward the new merge in the same writes, and reverted with them if those fail, so a retry converges on one live merged episode.
With the per-merge source cap a large cluster converges over several runs rather than in one. reflection_deprecated now carries cluster_remaining_count (members this merge left for a later run) and retired_orphan_count (merged episodes of failed runs retired here).
|
@BrierAinz Good catch. The edge was real, so I fixed it rather than just adding a note. Retry after a compensated failure. The failed run's merged episode stays live and outside the cluster. It can never be selected as a source, because sources come from cluster membership and the membership update is the step that never ran. But the retry wrote a second merged episode beside it. A test that makes the first deprecation write miss its lock deadline, lets compensation revert it, and then retries ended with two live merged episodes at the previous head: 9f6d620 moves orphan detection from the start of the run, where it only logged, into the deprecation step. Live merged episodes of the cluster that are neither current members nor this run's merge are deprecated toward the new merge in the same writes, and reverted with them if those fail, so a retry converges on one live merged episode. The orphan's markdown is patched like any source's: a merged episode is written to markdown first, so it always has an Telemetry for the cap. A capped run already logs |
This branch has not been deployed
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