Add atoms::Merger - #1205
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAtomDB insertion APIs now accept optional merger strategies instead of boolean duplicate flags. Backends implement replacement, duplicate rejection, custom batch merging, index updates, and transactional metadata handling. Callers and tests use the revised APIs. ChangesMerger-based AtomDB insertion
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant AtomDB
participant Backend
participant Merger
participant Storage
Caller->>AtomDB: add atom or batch
AtomDB->>Backend: forward transactional flag and merger
Backend->>Merger: merge existing and incoming atoms
Backend->>Storage: persist merged atom and indexes
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/atomdb/morkdb/MorkDB.cc (1)
245-293: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefer MORK writes until merger validation completes.
A batch with 5,000 successful links followed by a duplicate using
ThrowIfExistsMergerposts MORK at Line 279, then throws on the later merge; Mongo upsert at Lines 301-303 is never reached. This leaves MORK and Mongo inconsistent. Stage MORK submissions until all merges succeed, and add a late-failure regression test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/morkdb/MorkDB.cc` around lines 245 - 293, The link loop in the batch persistence flow submits MORK data before all merger operations have succeeded, allowing a later merge failure to leave MORK ahead of Mongo. In the code handling links and merger validation, accumulate metta expressions and defer every mork_client->post call until the full batch completes successfully, while preserving chunking only for staged data; add a regression test covering many successful links followed by a ThrowIfExistsMerger duplicate and verify no MORK submission occurs when merging fails.
🧹 Nitpick comments (3)
src/atomdb/inmemorydb/InMemoryDB.cc (2)
409-418: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winIndexing uses
link, not the mergedto_storeequivalent — diverges fromRedisMongoDB::add_links.
RedisMongoDBcomputesmatch_pattern_index_schema(to_store)and iteratesto_store->targets; here the incoming-set and pattern indexing always use the pre-mergelink. Harmless while mergers preserve targets, but the two backends now index differently for anything that doesn't. Worth aligning, or documenting inMergerthat mergers must not alter targets/type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/inmemorydb/InMemoryDB.cc` around lines 409 - 418, Align InMemoryDB indexing with RedisMongoDB by using the merged to_store link for both incoming-set updates and match_pattern_index_schema instead of the pre-merge link. Update the target iteration and pattern indexing in the surrounding link-storage flow, preserving the existing add_incoming_set and add_pattern calls.
320-325: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
catch (...)+RAISE_ERRORdiscards the merger's own error and hand-rolls cleanup.
ThrowIfExistsMerger's "Node already exists: " message is replaced by "Failed to merge node", so callers lose the reason. Aunique_ptrworking copy lets the original exception propagate with no manualdelete:- Node* working = new Node(*dynamic_cast<Node*>(atom_trie_value->get_atom())); - try { - merger->merge(working, node); - } catch (...) { - delete working; - RAISE_ERROR("Failed to merge node: " + node->handle()); - } - atoms_trie_->insert(handle, new AtomTrieValue(working)); + auto working = make_unique<Node>(*dynamic_cast<Node*>(atom_trie_value->get_atom())); + merger->merge(working.get(), node); // throws through; stored state untouched + atoms_trie_->insert(handle, new AtomTrieValue(working.release()));Same applies to the link path at Lines 400-405. See the contract note on
src/atomdb/AtomDB.h.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/inmemorydb/InMemoryDB.cc` around lines 320 - 325, Update the node merge path around merger->merge(working, node) to manage working with a unique_ptr and remove the catch-all handler, manual delete, and replacement RAISE_ERROR so the merger’s original exception propagates unchanged. Apply the same ownership and exception-propagation fix to the corresponding link merge path.src/tests/cpp/remote_atomdb_test.cc (1)
46-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise merger propagation through remote layers.
These tests only cover default insertion. Add duplicate-node/link cases through both
RemoteAtomDBPeerandRemoteAtomDBusingThrowIfExistsMerger; otherwise a dropped merger argument can silently become an upsert.As per path instructions, tests should validate “real behavior” and proxy interactions.
Also applies to: 412-414
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/cpp/remote_atomdb_test.cc` around lines 46 - 51, Extend RemoteAtomDBPeerTest cases around AddAndGetNodes and the corresponding link tests to insert duplicate nodes and links through both RemoteAtomDBPeer and RemoteAtomDB, passing ThrowIfExistsMerger. Assert duplicates raise the expected error so merger propagation through each proxy layer is verified rather than allowing an unintended upsert.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/atomdb/AtomDB.h`:
- Around line 45-53: Standardize merger exception behavior across InMemoryDB and
RedisMongoDB by choosing either direct propagation or consistent wrapping, then
update add_node/add_nodes/add_link/add_links implementations accordingly.
Document the chosen guarantee in the add_atom/add_node/add_link contract near
the Merger behavior, preserving the merger’s expected caller-visible semantics
across backends.
In `@src/atomdb/redis_mongodb/RedisMongoDB.cc`:
- Around line 807-817: The merge path in RedisMongoDB::add_node performs an
unsafe read-modify-write that can lose concurrent merges. Serialize
merger-enabled updates per node handle, or implement an atomic MongoDB
findOneAndUpdate with optimistic revision checking and retry; apply the same
protection to add_nodes and add_links, preserving non-merge insert behavior.
- Around line 871-893: Update the node batch path around batch_merged so merging
still processes every occurrence but serializes exactly one MongodbDocument per
unique handle after the loop. Apply the same deduplication to the link batch
path at src/atomdb/redis_mongodb/RedisMongoDB.cc lines 927-956, emitting one
merged link per handle and skipping repeated incoming-set/pattern ZADD appends
so duplicate occurrences do not consume additional scores.
In `@src/tests/cpp/chain_operator_test.cc`:
- Around line 57-62: Update the duplicate insertion assertion in the node setup
around db->add_node to retain the newly allocated duplicate atoms::Node in a
local pointer, assert its handle, and delete it afterward. Preserve the existing
evaluation node and duplicate-tolerant behavior while ensuring both allocations
are released.
---
Outside diff comments:
In `@src/atomdb/morkdb/MorkDB.cc`:
- Around line 245-293: The link loop in the batch persistence flow submits MORK
data before all merger operations have succeeded, allowing a later merge failure
to leave MORK ahead of Mongo. In the code handling links and merger validation,
accumulate metta expressions and defer every mork_client->post call until the
full batch completes successfully, while preserving chunking only for staged
data; add a regression test covering many successful links followed by a
ThrowIfExistsMerger duplicate and verify no MORK submission occurs when merging
fails.
---
Nitpick comments:
In `@src/atomdb/inmemorydb/InMemoryDB.cc`:
- Around line 409-418: Align InMemoryDB indexing with RedisMongoDB by using the
merged to_store link for both incoming-set updates and
match_pattern_index_schema instead of the pre-merge link. Update the target
iteration and pattern indexing in the surrounding link-storage flow, preserving
the existing add_incoming_set and add_pattern calls.
- Around line 320-325: Update the node merge path around merger->merge(working,
node) to manage working with a unique_ptr and remove the catch-all handler,
manual delete, and replacement RAISE_ERROR so the merger’s original exception
propagates unchanged. Apply the same ownership and exception-propagation fix to
the corresponding link merge path.
In `@src/tests/cpp/remote_atomdb_test.cc`:
- Around line 46-51: Extend RemoteAtomDBPeerTest cases around AddAndGetNodes and
the corresponding link tests to insert duplicate nodes and links through both
RemoteAtomDBPeer and RemoteAtomDB, passing ThrowIfExistsMerger. Assert
duplicates raise the expected error so merger propagation through each proxy
layer is verified rather than allowing an unintended upsert.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 14be289e-3dd4-4899-bc94-d9cb153c2df0
📒 Files selected for processing (30)
src/agents/atomdb_broker/AtomDBProxy.ccsrc/agents/link_creation_agent/MettaTemplateProcessor.ccsrc/atomdb/AtomDB.hsrc/atomdb/BUILDsrc/atomdb/adapterdb/AdapterDB.ccsrc/atomdb/adapterdb/AdapterDB.hsrc/atomdb/inmemorydb/InMemoryDB.ccsrc/atomdb/inmemorydb/InMemoryDB.hsrc/atomdb/morkdb/MorkDB.ccsrc/atomdb/morkdb/MorkDB.hsrc/atomdb/redis_mongodb/RedisMongoDB.ccsrc/atomdb/redis_mongodb/RedisMongoDB.hsrc/atomdb/remotedb/RemoteAtomDB.ccsrc/atomdb/remotedb/RemoteAtomDB.hsrc/atomdb/remotedb/RemoteAtomDBPeer.ccsrc/atomdb/remotedb/RemoteAtomDBPeer.hsrc/commons/atoms/BUILDsrc/commons/atoms/Merger.hsrc/db_adapter/AtomPersister.ccsrc/main/db_loader.ccsrc/tests/cpp/adapterdb_test.ccsrc/tests/cpp/chain_operator_test.ccsrc/tests/cpp/inmemorydb_test.ccsrc/tests/cpp/link_creation_agent_test.ccsrc/tests/cpp/morkdb_test.ccsrc/tests/cpp/redis_mongodb_test.ccsrc/tests/cpp/redis_mongodb_test_2.ccsrc/tests/cpp/remote_atomdb_test.ccsrc/tests/cpp/test_commons/mocks/MockAtomDB.hsrc/tests/main/evaluation_evolution.cc
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tests/cpp/morkdb_test.cc (1)
344-352: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
add_nodes/add_links(…, false, true)was collapsed to(…, true)in three tests. The old third argument wasthrow_if_exists, notis_transactional. Dropping it promotedis_transactionalfromfalsetotrueand removed duplicate rejection, so these tests now exercise the transactional composite-type path with plain upserts instead of what they were written to cover.
src/tests/cpp/morkdb_test.cc#L344-L352: restorefalseforis_transactionaland pass&ThrowIfExistsMerger::instance()in both the in-loop flush and the post-loop flush.src/tests/cpp/redis_mongodb_test.cc#L1240-L1241: restore the originalis_transactionalvalue forCompositeTypeEnabledFlagand pass&ThrowIfExistsMerger::instance()for the duplicate-rejection intent.src/tests/cpp/redis_mongodb_test_2.cc#L244-L252: same two-line fix in the in-loop and post-loopdb2->add_nodes/add_linkscalls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/cpp/morkdb_test.cc` around lines 344 - 352, Restore the original add_nodes/add_links argument semantics: in src/tests/cpp/morkdb_test.cc lines 344-352, update both in-loop and post-loop flushes to use is_transactional=false and ThrowIfExistsMerger::instance(); apply the same duplicate-rejection merger and original CompositeTypeEnabledFlag is_transactional value in src/tests/cpp/redis_mongodb_test.cc lines 1240-1241, and both db2 flushes in src/tests/cpp/redis_mongodb_test_2.cc lines 244-252.
🧹 Nitpick comments (5)
src/tests/cpp/inmemorydb_test.cc (1)
690-714: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test covers
merge()returningfalse. That's the branch thatMerger.hdocuments most emphatically ("backends must not persist the working copy") and it's implemented inInMemoryDB::add_node/add_linksvia thedelete working; return/continuepaths — currently untested in every test file in this PR. A tinyRejectMerger { bool merge(...) const override { return false; } }plus assertions that the stored atom keeps its original attributes (and, for links, that incoming-set/pattern queries are unchanged) would lock the contract down.Want me to draft those cases?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/cpp/inmemorydb_test.cc` around lines 690 - 714, Add tests using a RejectMerger whose merge method returns false, covering both InMemoryDB::add_node and InMemoryDB::add_links. Assert rejected node merges preserve the stored atom’s original attributes, and rejected link merges leave incoming-set and pattern-query results unchanged; retain the existing successful and throwing merger tests.src/atomdb/inmemorydb/InMemoryDB.cc (1)
388-406: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSkipping index updates on a failed merge is the right call — worth a brief note.
The
continuecorrectly bypasses incoming-set/pattern indexing since the link was already indexed by whichever add created it. One nit on ownership: the two manualdelete workingpaths could collapse into aunique_ptr<Link>plusrelease()on the insert, removing thecatch (...)entirely. Optional, and only if it matches the surrounding style.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/inmemorydb/InMemoryDB.cc` around lines 388 - 406, In the merge branch of the InMemoryDB update flow, replace the manual ownership and cleanup of working with a unique_ptr<Link>, releasing it only when atoms_trie_->insert succeeds. Remove the redundant catch block while preserving the existing behavior: failed merges skip insertion and indexing, and exceptions clean up automatically.src/atomdb/redis_mongodb/RedisMongoDB.cc (1)
955-986: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftBatch merge issues one
get_linkround-trip per unique handle. For a merger-enabledadd_linksof N distinct handles this is N sequential Mongo lookups before a single bulk upsert, which will dominate ingestion time on large batches. The existingget_atom_documents(...)bulk path could pre-fetch the stored links once and feed the merge loop from a local map. Same shape applies toadd_nodesabove.Not a blocker for this PR, but worth a follow-up if merger-enabled bulk ingestion is on a hot path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/redis_mongodb/RedisMongoDB.cc` around lines 955 - 986, The merger-enabled batch paths in add_links and add_nodes perform one sequential get_link lookup per unique handle. Follow up by using the existing get_atom_documents(...) bulk-fetch mechanism to load stored links once into a local handle-to-link map, then have the merge loops reuse that map instead of calling get_link for each handle while preserving current merge and persistence behavior.src/atomdb/morkdb/MorkDB.cc (2)
285-316: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
const_castto writemetta_expressiononto the stored link. For themerger == NULLpathto_storeis still the caller'sLink*, so this silently mutates the caller's object — same as before this PR, but theconst_castnow makes it look deliberate. Sincelinks_to_persistisvector<const atoms::Link*>, consider holdingatoms::Link*instead and dropping the cast, or documenting that callers must expectmetta_expressionto be filled in.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/morkdb/MorkDB.cc` around lines 285 - 316, The persistence loop in MorkDB currently uses const_cast to mutate links from links_to_persist, including caller-owned links in the merger == NULL path. Remove this implicit mutation by changing the collection or flow to use mutable atoms::Link* where appropriate and assign metta_expression without const_cast, or explicitly document the required caller-visible mutation if constness must remain.
247-282: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftThis batch-merge block is a near-verbatim copy of
RedisMongoDB::add_links/add_nodes. Three backends now carry the same dedup-into-batch_merged+unique_handles+links_to_persistdance, so any future fix to merge semantics has to be applied in three places. A small shared helper (e.g. inAtomDBor alongsideMerger) taking the incoming vector, a "fetch existing" callable, and the merger, returning the list to persist, would keep the semantics in one spot.Deferrable — flagging it now because the merge contract is likely to keep evolving.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/morkdb/MorkDB.cc` around lines 247 - 282, Extract the duplicated batch-merge logic from MorkDB’s link persistence flow, centered on the merger, batch_merged, unique_handles, and links_to_persist handling, into a shared helper in AtomDB or alongside Merger. Have the helper accept incoming links, an existing-link lookup callable, and the merger, then return the links to persist while preserving current merge and deduplication behavior; update MorkDB and the corresponding RedisMongoDB add_links/add_nodes paths to use it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/tests/cpp/morkdb_test.cc`:
- Around line 344-352: Restore the original add_nodes/add_links argument
semantics: in src/tests/cpp/morkdb_test.cc lines 344-352, update both in-loop
and post-loop flushes to use is_transactional=false and
ThrowIfExistsMerger::instance(); apply the same duplicate-rejection merger and
original CompositeTypeEnabledFlag is_transactional value in
src/tests/cpp/redis_mongodb_test.cc lines 1240-1241, and both db2 flushes in
src/tests/cpp/redis_mongodb_test_2.cc lines 244-252.
---
Nitpick comments:
In `@src/atomdb/inmemorydb/InMemoryDB.cc`:
- Around line 388-406: In the merge branch of the InMemoryDB update flow,
replace the manual ownership and cleanup of working with a unique_ptr<Link>,
releasing it only when atoms_trie_->insert succeeds. Remove the redundant catch
block while preserving the existing behavior: failed merges skip insertion and
indexing, and exceptions clean up automatically.
In `@src/atomdb/morkdb/MorkDB.cc`:
- Around line 285-316: The persistence loop in MorkDB currently uses const_cast
to mutate links from links_to_persist, including caller-owned links in the
merger == NULL path. Remove this implicit mutation by changing the collection or
flow to use mutable atoms::Link* where appropriate and assign metta_expression
without const_cast, or explicitly document the required caller-visible mutation
if constness must remain.
- Around line 247-282: Extract the duplicated batch-merge logic from MorkDB’s
link persistence flow, centered on the merger, batch_merged, unique_handles, and
links_to_persist handling, into a shared helper in AtomDB or alongside Merger.
Have the helper accept incoming links, an existing-link lookup callable, and the
merger, then return the links to persist while preserving current merge and
deduplication behavior; update MorkDB and the corresponding RedisMongoDB
add_links/add_nodes paths to use it.
In `@src/atomdb/redis_mongodb/RedisMongoDB.cc`:
- Around line 955-986: The merger-enabled batch paths in add_links and add_nodes
perform one sequential get_link lookup per unique handle. Follow up by using the
existing get_atom_documents(...) bulk-fetch mechanism to load stored links once
into a local handle-to-link map, then have the merge loops reuse that map
instead of calling get_link for each handle while preserving current merge and
persistence behavior.
In `@src/tests/cpp/inmemorydb_test.cc`:
- Around line 690-714: Add tests using a RejectMerger whose merge method returns
false, covering both InMemoryDB::add_node and InMemoryDB::add_links. Assert
rejected node merges preserve the stored atom’s original attributes, and
rejected link merges leave incoming-set and pattern-query results unchanged;
retain the existing successful and throwing merger tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 140314cf-a2bf-4e15-9bd9-f00007f9af67
📒 Files selected for processing (10)
src/atomdb/AtomDB.hsrc/atomdb/inmemorydb/InMemoryDB.ccsrc/atomdb/morkdb/MorkDB.ccsrc/atomdb/redis_mongodb/RedisMongoDB.ccsrc/commons/atoms/Merger.hsrc/tests/cpp/chain_operator_test.ccsrc/tests/cpp/inmemorydb_test.ccsrc/tests/cpp/morkdb_test.ccsrc/tests/cpp/redis_mongodb_test.ccsrc/tests/cpp/redis_mongodb_test_2.cc
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tests/cpp/chain_operator_test.cc
- src/atomdb/AtomDB.h
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/atomdb/redis_mongodb/RedisMongoDB.cc (2)
1018-1022: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMerged links persist stale composite-type metadata in both backends.
Compute composite-type entries and hashes from the final merged object, not from the original input batch:
src/atomdb/redis_mongodb/RedisMongoDB.cc#L1018-L1022: derive transactional metadata from each finalto_storelink.src/atomdb/morkdb/MorkDB.cc#L309-L312: apply the same fix using the final merged link.As per path instructions, behavior changes under
src/require matching*_test.cccoverage, including edge cases and error paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/redis_mongodb/RedisMongoDB.cc` around lines 1018 - 1022, Update the transactional metadata construction at RedisMongoDB.cc:1018-1022 and the corresponding MorkDB.cc:309-312 logic to derive composite-type entries and hashes from each final merged to_store link, not the original input batch. Add matching *_test.cc coverage under src/ for merged-link metadata, including relevant edge and error paths.Source: Path instructions
994-1012: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRemove stale Redis index memberships during link upserts.
This path only adds current target/pattern memberships with
ZADD; it never removes memberships belonging to the previously stored link. Replacing or merging a link with different targets or pattern handles can therefore make queries return the link under obsolete targets/patterns even though MongoDB contains the new link.Capture the old index memberships and issue the corresponding removals, or rebuild the affected Redis indexes atomically. Add regression coverage for target and pattern replacement.
As per path instructions, behavior changes under
src/require matching*_test.cccoverage, including edge cases and error paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/redis_mongodb/RedisMongoDB.cc` around lines 994 - 1012, Update the link upsert path around to_store->targets and pattern_handles to remove stale Redis ZSET memberships from the previously stored link before or atomically with adding current memberships. Capture the old target and pattern index entries, issue corresponding ZREM operations, and preserve current ZADD behavior for replacements and merges. Add matching *_test.cc regression coverage for target and pattern replacement, including relevant edge and error paths.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/atomdb/redis_mongodb/RedisMongoDB.cc`:
- Around line 1018-1022: Update the transactional metadata construction at
RedisMongoDB.cc:1018-1022 and the corresponding MorkDB.cc:309-312 logic to
derive composite-type entries and hashes from each final merged to_store link,
not the original input batch. Add matching *_test.cc coverage under src/ for
merged-link metadata, including relevant edge and error paths.
- Around line 994-1012: Update the link upsert path around to_store->targets and
pattern_handles to remove stale Redis ZSET memberships from the previously
stored link before or atomically with adding current memberships. Capture the
old target and pattern index entries, issue corresponding ZREM operations, and
preserve current ZADD behavior for replacements and merges. Add matching
*_test.cc regression coverage for target and pattern replacement, including
relevant edge and error paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f36287a-aa93-4f42-9b1e-4b4b2d5e719f
📒 Files selected for processing (2)
src/atomdb/morkdb/MorkDB.ccsrc/atomdb/redis_mongodb/RedisMongoDB.cc
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/atomdb/morkdb/MorkDB.cc (1)
312-321: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDerive transactional
composite_type_hashfromcomposite_type_entries_mapin MorkDB.
MorkDB::build_composite_type_entries_mapis inherited fromRedisMongoDB, wherecomposite_type_hashes_map[link_handle]is set to the link’s named type hash, making MorkDB storenamed_type_hashwhilecomposite_type_entries_mapcontains the recursive composite type. UseHasher::composite_handle(composite_type_entries_map[link_handle])for the stored transactional document before upserting, matching RedisMongoDB and the intendedcomposite_typeshape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/morkdb/MorkDB.cc` around lines 312 - 321, In the transactional branch of the document construction flow, update the value passed to the MorkDB `mongodb_doc.emplace` call so the stored composite type hash is derived with `Hasher::composite_handle(composite_type_entries_map[link_handle])` instead of `composite_type_hashes_map_copy[link_handle]`. Leave the non-transactional and empty-composite branches unchanged.src/atomdb/redis_mongodb/RedisMongoDB.cc (1)
948-996: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFix composite-type metadata for transactional rejected merges.
add_links()should not skip composite-type bookkeeping whenmerge()returns false. In the existingTxRejectCTtest, a later link in the same transactional batch stores a default empty target hash for the rejected duplicate link, so itscomposite_typearray has an extra bogus entry beyond the fresh link’s expected 4 target hashes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/redis_mongodb/RedisMongoDB.cc` around lines 948 - 996, Update add_links() so a failed merger->merge() does not bypass composite-type bookkeeping: retain the rejected link for build_composite_type_entries_map while keeping it excluded from links_to_persist. Ensure metadata is derived from both persisted final objects and rejected transactional links without changing persistence behavior.
🧹 Nitpick comments (3)
src/atomdb/morkdb/MorkDB.cc (1)
279-287: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCopying the whole
composite_type_hashes_mapjust to read a handful of handles.
composite_type_hashes_mapaccumulates an entry per atom seen in the transaction, so this snapshot grows with load size while only thelinks_to_persisthandles are ever read back at Line 315. Copy just those, still under the lock.♻️ Proposed refactor
if (this->composite_type_enabled() && is_transactional) { this->build_composite_type_entries_map(links_to_persist, composite_type_entries_map); lock_guard<mutex> composite_type_hashes_map_lock(this->composite_type_hashes_map_mutex); - composite_type_hashes_map_copy = this->composite_type_hashes_map; + for (const auto* link : links_to_persist) { + const string handle = link->handle(); + auto it = this->composite_type_hashes_map.find(handle); + if (it != this->composite_type_hashes_map.end()) { + composite_type_hashes_map_copy[handle] = it->second; + } + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/morkdb/MorkDB.cc` around lines 279 - 287, Update the transactional metadata setup around composite_type_hashes_map_copy to copy only entries whose handles are present in links_to_persist, while holding composite_type_hashes_map_mutex. Preserve the existing lookup behavior at the later read site and avoid snapshotting the entire composite_type_hashes_map.src/atomdb/redis_mongodb/RedisMongoDB.cc (2)
948-983: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftThe merge/batch-dedup loop is duplicated verbatim across two backends. Both files implement the identical
batch_merged+unique_handlesread-merge-dedup algorithm (including the "failed merge → continue" rule), so any future fix to the merge contract has to be applied twice and will silently drift.
src/atomdb/redis_mongodb/RedisMongoDB.cc#L948-L983: extract this loop into a shared helper (e.g. an AtomDB-levelmerge_batch(links, merger, out_handles)returning the deduped working copies) and call it here.src/atomdb/morkdb/MorkDB.cc#L242-L277: replace the copy with a call to the same helper, keeping the local non-constlinks_to_persistfor the metta_expression fill-in.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/redis_mongodb/RedisMongoDB.cc` around lines 948 - 983, Extract the duplicated batch merge/deduplication logic from RedisMongoDB.cc lines 948-983 and MorkDB.cc lines 242-277 into a shared AtomDB-level helper, such as merge_batch, preserving the batch_merged and unique_handles behavior and the failed-merge continue rule. Replace both loops with calls to this helper; in MorkDB.cc, retain the local non-const links_to_persist handling required for metta_expression fill-in.
985-996: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the shadow vector +
const_cast: declarelinks_to_persistasvector<atoms::Link*>.
MorkDB::add_links(line 240 ofsrc/atomdb/morkdb/MorkDB.cc) already stores non-constatoms::Link*and passes the container straight through. Matching it here removes an extra per-batch allocation and theconst_cast.♻️ Proposed refactor
- vector<const atoms::Link*> links_to_persist; + vector<atoms::Link*> links_to_persist; @@ - map<string, vector<string>> composite_type_entries_map; - if (this->composite_type_enabled_ && is_transactional) { - vector<atoms::Link*> links_for_composite; - links_for_composite.reserve(links_to_persist.size()); - for (const auto* link : links_to_persist) { - links_for_composite.push_back(const_cast<atoms::Link*>(link)); - } - this->build_composite_type_entries_map(links_for_composite, composite_type_entries_map); - } + map<string, vector<string>> composite_type_entries_map; + if (this->composite_type_enabled_ && is_transactional) { + this->build_composite_type_entries_map(links_to_persist, composite_type_entries_map); + }(
links_to_persist.reserve(links.size())before the loop would also avoid the incremental growth.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/redis_mongodb/RedisMongoDB.cc` around lines 985 - 996, Declare links_to_persist as vector<atoms::Link*> and pass it directly to build_composite_type_entries_map in the transactional composite-type path. Remove the temporary links_for_composite vector, its reserve call, and the const_cast conversion; also reserve links_to_persist from the source links before populating it if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/atomdb/morkdb/MorkDB.cc`:
- Around line 312-321: In the transactional branch of the document construction
flow, update the value passed to the MorkDB `mongodb_doc.emplace` call so the
stored composite type hash is derived with
`Hasher::composite_handle(composite_type_entries_map[link_handle])` instead of
`composite_type_hashes_map_copy[link_handle]`. Leave the non-transactional and
empty-composite branches unchanged.
In `@src/atomdb/redis_mongodb/RedisMongoDB.cc`:
- Around line 948-996: Update add_links() so a failed merger->merge() does not
bypass composite-type bookkeeping: retain the rejected link for
build_composite_type_entries_map while keeping it excluded from
links_to_persist. Ensure metadata is derived from both persisted final objects
and rejected transactional links without changing persistence behavior.
---
Nitpick comments:
In `@src/atomdb/morkdb/MorkDB.cc`:
- Around line 279-287: Update the transactional metadata setup around
composite_type_hashes_map_copy to copy only entries whose handles are present in
links_to_persist, while holding composite_type_hashes_map_mutex. Preserve the
existing lookup behavior at the later read site and avoid snapshotting the
entire composite_type_hashes_map.
In `@src/atomdb/redis_mongodb/RedisMongoDB.cc`:
- Around line 948-983: Extract the duplicated batch merge/deduplication logic
from RedisMongoDB.cc lines 948-983 and MorkDB.cc lines 242-277 into a shared
AtomDB-level helper, such as merge_batch, preserving the batch_merged and
unique_handles behavior and the failed-merge continue rule. Replace both loops
with calls to this helper; in MorkDB.cc, retain the local non-const
links_to_persist handling required for metta_expression fill-in.
- Around line 985-996: Declare links_to_persist as vector<atoms::Link*> and pass
it directly to build_composite_type_entries_map in the transactional
composite-type path. Remove the temporary links_for_composite vector, its
reserve call, and the const_cast conversion; also reserve links_to_persist from
the source links before populating it if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f7159360-8daa-4bfa-b1b3-a56c3a51be18
📒 Files selected for processing (3)
src/atomdb/morkdb/MorkDB.ccsrc/atomdb/redis_mongodb/RedisMongoDB.ccsrc/tests/cpp/redis_mongodb_test.cc
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/atomdb/inmemorydb/InMemoryDB.cc (1)
354-384: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDuplicate handles inside one batch bypass the all-or-nothing precheck.
nodes_exist(handles)only tests the store. If the same handle appears twice innodes, the precheck passes. The firstadd_nodeinserts it, and the second call reaches the merge branch withThrowIfExistsMerger, which raises. The batch then throws after a partial insert, so the comment on Line 367 no longer holds. The same gap exists inadd_linksat Lines 386-409.Consider detecting repeated handles in the precheck as well.
🛡️ Proposed precheck extension
if (merger == &ThrowIfExistsMerger::instance()) { + set<string> seen; + for (const auto& handle : handles) { + if (!seen.insert(handle).second) { + RAISE_ERROR("Failed to insert nodes, duplicated node in batch: " + handle); + return {}; + } + } auto existing_handles = this->nodes_exist(handles);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/inmemorydb/InMemoryDB.cc` around lines 354 - 384, Extend the all-or-nothing prechecks in InMemoryDB::add_nodes and add_links to detect duplicate handles within the incoming batch, not only handles already stored. Treat any repeated handle as a conflict and raise before invoking add_node or the corresponding link insertion, preserving zero partial inserts for ThrowIfExistsMerger.
🧹 Nitpick comments (3)
src/tests/cpp/inmemorydb_test.cc (1)
838-888: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd the in-batch duplicate case to these all-or-nothing tests.
Both tests only cover a handle that already exists in the store. Add a case where the duplicate exists only inside the batch, for example
db->add_nodes({a, a_copy}, false, &ThrowIfExistsMerger::instance()). That case currently escapes thenodes_existprecheck and reveals the partial-insert path described insrc/atomdb/inmemorydb/InMemoryDB.ccLines 354-384.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/cpp/inmemorydb_test.cc` around lines 838 - 888, Extend AddNodesThrowIfExistsIsAllOrNothing and AddLinksThrowIfExistsIsAllOrNothing with batches containing two objects sharing the same handle, where that handle is absent from the database beforehand. Assert that ThrowIfExistsMerger raises runtime_error and that neither duplicate nor any other batch item remains inserted, matching the existing all-or-nothing assertions.src/atomdb/redis_mongodb/RedisMongoDB.cc (1)
874-904: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnnecessary copy of the fetched atom on every batch merge path.
get_node()andget_link()return freshly built, detached objects that no other code owns, so cloning them before callingmerger->merge()duplicates a fullPropertiesmap (and, for links, the target vector) once per unique handle.RedisMongoDB::add_nodeat Line 813 already merges into the fetched object directly; the batch paths should do the same.
src/atomdb/redis_mongodb/RedisMongoDB.cc#L874-L904: dropmake_shared<Node>(*existing_node)and merge intoexisting_node, storing it inbatch_merged.src/atomdb/redis_mongodb/RedisMongoDB.cc#L964-L1008: dropmake_shared<Link>(*existing_link)and merge intoexisting_link, storing it inbatch_merged,composite_keepalive, andlinks_for_composite.src/atomdb/morkdb/MorkDB.cc#L244-L285: apply the same change to theexisting_linkclone at Line 259.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/redis_mongodb/RedisMongoDB.cc` around lines 874 - 904, Remove the unnecessary clones before merge operations: in RedisMongoDB.cc lines 874-904, merge directly into existing_node and store it in batch_merged; in RedisMongoDB.cc lines 964-1008, merge directly into existing_link and store it in batch_merged, composite_keepalive, and links_for_composite; in MorkDB.cc lines 244-285, apply the same direct merge to existing_link. Preserve the existing failure handling and ownership flows.Source: Path instructions
src/atomdb/remotedb/RemoteAtomDB.cc (1)
330-383: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
this->remote_db_for each fan-out loop.The changed methods access the
RemoteAtomDB::remote_db_member withoutthis->. Update all six loops to usethis->remote_db_consistently.Proposed fix
- for (auto& [uid, peer] : remote_db_) { + for (auto& [uid, peer] : this->remote_db_) {Apply this change at Lines 330, 339, 348, 359, 370, and 381.
As per coding guidelines, “Access class members with
this->fieldconsistently in C++.” As per path instructions, “Usethis->for member access.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/remotedb/RemoteAtomDB.cc` around lines 330 - 383, Update the fan-out loops in RemoteAtomDB::add_atom, add_node, add_link, add_atoms, add_nodes, and add_links to iterate over this->remote_db_ instead of remote_db_. Preserve the existing loop bodies and behavior.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/atomdb/AtomDB.h`:
- Around line 58-66: Update all batch add_atoms, add_nodes, and add_links call
sites to use the three-argument form explicitly: pass the intended
is_transactional value and either the appropriate merger instance, such as
ThrowIfExistsMerger::instance(), or nullptr. Do not leave two-argument calls
that could silently change duplicate-handling behavior.
---
Outside diff comments:
In `@src/atomdb/inmemorydb/InMemoryDB.cc`:
- Around line 354-384: Extend the all-or-nothing prechecks in
InMemoryDB::add_nodes and add_links to detect duplicate handles within the
incoming batch, not only handles already stored. Treat any repeated handle as a
conflict and raise before invoking add_node or the corresponding link insertion,
preserving zero partial inserts for ThrowIfExistsMerger.
---
Nitpick comments:
In `@src/atomdb/redis_mongodb/RedisMongoDB.cc`:
- Around line 874-904: Remove the unnecessary clones before merge operations: in
RedisMongoDB.cc lines 874-904, merge directly into existing_node and store it in
batch_merged; in RedisMongoDB.cc lines 964-1008, merge directly into
existing_link and store it in batch_merged, composite_keepalive, and
links_for_composite; in MorkDB.cc lines 244-285, apply the same direct merge to
existing_link. Preserve the existing failure handling and ownership flows.
In `@src/atomdb/remotedb/RemoteAtomDB.cc`:
- Around line 330-383: Update the fan-out loops in RemoteAtomDB::add_atom,
add_node, add_link, add_atoms, add_nodes, and add_links to iterate over
this->remote_db_ instead of remote_db_. Preserve the existing loop bodies and
behavior.
In `@src/tests/cpp/inmemorydb_test.cc`:
- Around line 838-888: Extend AddNodesThrowIfExistsIsAllOrNothing and
AddLinksThrowIfExistsIsAllOrNothing with batches containing two objects sharing
the same handle, where that handle is absent from the database beforehand.
Assert that ThrowIfExistsMerger raises runtime_error and that neither duplicate
nor any other batch item remains inserted, matching the existing all-or-nothing
assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c41cbc3-c114-4238-ac8e-a78b76b87364
📒 Files selected for processing (30)
src/agents/atomdb_broker/AtomDBProxy.ccsrc/agents/link_creation_agent/MettaTemplateProcessor.ccsrc/atomdb/AtomDB.hsrc/atomdb/BUILDsrc/atomdb/adapterdb/AdapterDB.ccsrc/atomdb/adapterdb/AdapterDB.hsrc/atomdb/inmemorydb/InMemoryDB.ccsrc/atomdb/inmemorydb/InMemoryDB.hsrc/atomdb/morkdb/MorkDB.ccsrc/atomdb/morkdb/MorkDB.hsrc/atomdb/redis_mongodb/RedisMongoDB.ccsrc/atomdb/redis_mongodb/RedisMongoDB.hsrc/atomdb/remotedb/RemoteAtomDB.ccsrc/atomdb/remotedb/RemoteAtomDB.hsrc/atomdb/remotedb/RemoteAtomDBPeer.ccsrc/atomdb/remotedb/RemoteAtomDBPeer.hsrc/commons/atoms/BUILDsrc/commons/atoms/Merger.hsrc/db_adapter/AtomPersister.ccsrc/main/db_loader.ccsrc/tests/cpp/adapterdb_test.ccsrc/tests/cpp/chain_operator_test.ccsrc/tests/cpp/inmemorydb_test.ccsrc/tests/cpp/link_creation_agent_test.ccsrc/tests/cpp/morkdb_test.ccsrc/tests/cpp/redis_mongodb_test.ccsrc/tests/cpp/redis_mongodb_test_2.ccsrc/tests/cpp/remote_atomdb_test.ccsrc/tests/cpp/test_commons/mocks/MockAtomDB.hsrc/tests/main/evaluation_evolution.cc
2 participants
Summary
Replace
bool throw_if_existson AtomDBadd_*APIs with an optionalatoms::Mergerstrategy.merger == nullptr(default): upsert — insert if missing, replace if presentMerger: merge into a working copy, persist only on successThrowIfExistsMerger: rejects duplicates (formerthrow_if_exists=true)Wired through InMemoryDB, RedisMongoDB, MorkDB, RemoteAtomDB/Peer, and AdapterDB. Call sites and tests updated accordingly.
Test plan
inmemorydb_test— replace / throw / custom merge for nodes and linksredis_mongodb_test— single + batch replace/merge; ThrowIfExists pathsadapterdb_test/chain_operator_test/ related agent tests still pass