[Toy] Fix RemoteDB concurrent logic#1189
Conversation
WalkthroughInMemoryDB gains synchronized operations, deep-copy enumeration, reset/drop APIs, and upsert behavior. Remote peers now use replaceable caches with staged persistence and dependency-aware flushing. Federation reads, writes, deletions, and cache release semantics are updated, with expanded tests and runtime configuration. ChangesStorage and persistence
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RemoteAtomDB
participant RemoteAtomDBPeer
participant InMemoryDB
participant AtomDB
Client->>RemoteAtomDB: get_atom(handle)
RemoteAtomDB->>RemoteAtomDBPeer: probe writable and readonly peers
RemoteAtomDBPeer->>InMemoryDB: read cache
RemoteAtomDBPeer->>AtomDB: fetch on cache miss
AtomDB-->>RemoteAtomDBPeer: return atom
RemoteAtomDBPeer->>InMemoryDB: warm cache
Client->>RemoteAtomDB: release_caches(schema)
RemoteAtomDB->>RemoteAtomDBPeer: release(schema)
RemoteAtomDBPeer->>AtomDB: persist staged atoms
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)config/das.jsonTraceback (most recent call last): src/tests/assets/remotedb_config_single.jsonTraceback (most recent call last): 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/atomdb/remotedb/RemoteAtomDB.cc (1)
415-440: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a multi-peer batch-delete test.
maxmatches the replicated-peer model here, butremote_atomdb_test.ccdoesn’t coverdelete_atoms/delete_nodes/delete_linksacross multiple peers, so the count semantics aren’t pinned down.🤖 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 415 - 440, Add a multi-peer batch-delete test in remote_atomdb_test.cc covering delete_atoms, delete_nodes, and delete_links through RemoteAtomDB with multiple replicated peers. Assert that each method returns the maximum deletion count reported by any peer, preserving the existing max-based semantics.src/atomdb/inmemorydb/InMemoryDB.cc (1)
190-208: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winCritical:
get_matching_atomswraps trie-ownedAtom*in a freshshared_ptr, causing a double-free/use-after-free.
shared_ptr<Node>(dynamic_cast<Node*>(atom))(and theLinkbranch) take ownership of the same raw pointer still owned byAtomTrieValue. When either the returnedshared_ptror~AtomTrieValue()runs first, the other holder is left with a dangling pointer, and the second destruction double-frees.get_atom()andget_all_atoms()were just fixed to return safe deep clones via the newclone_atom()helper — this method was left with the pre-existing unsafe pattern despite the added locking, which only serializes access to (but doesn't fix) the unsafe aliasing. Given this PR's explicit goal is eliminating concurrency-related crashes, this is a live segfault/UB risk on this hot path.🐛 Proposed fix using the new `clone_atom()` helper
Atom* atom = atom_trie_value->get_atom(); - if (Atom::is_node(*atom)) { - matching_atoms.push_back(shared_ptr<Node>(dynamic_cast<Node*>(atom))); - } else { - matching_atoms.push_back(shared_ptr<Link>(dynamic_cast<Link*>(atom))); - } + auto cloned = clone_atom(atom); + if (cloned != nullptr) { + matching_atoms.push_back(cloned); + } return matching_atoms;🤖 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 190 - 208, Update InMemoryDB::get_matching_atoms to return deep clones of the trie-owned atom using the existing clone_atom() helper instead of constructing shared_ptr<Node> or shared_ptr<Link> from the raw Atom*. Preserve the node/link result behavior while ensuring the returned pointers have independent ownership from AtomTrieValue.src/tests/cpp/inmemorydb_test.cc (1)
546-737: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftNo concurrency test for the newly synchronized
InMemoryDB.All new tests (
GetAllAtoms,DropAll*,UpsertReplacesCustomAttributes) are single-threaded. The PR's stated motivation is fixing segfaults from concurrent races on this exact class, and the header now documents it as thread-safe — but nothing here exercises multiple threads callingadd_link/get_atom/drop_all/query_for_patternconcurrently to validate that claim. As per path instructions, tests here should prioritize "thread/proxy interactions" for behavior changes like this.Want me to draft a stress test that spawns several threads doing concurrent
add_link/get_atom/delete_atomagainst a sharedInMemoryDBand asserts no crash/inconsistent state underThreadSanitizer?🤖 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 546 - 737, Extend the InMemoryDB tests with a multithreaded stress test that shares one database across threads and concurrently exercises add_link, get_atom, drop_all or delete_atom, and query_for_pattern. Synchronize thread start and completion, run enough iterations to expose races, and assert only safe post-join invariants such as a valid atom count or empty state; ensure the test is suitable for ThreadSanitizer and does not dereference results invalidated by concurrent drop_all.Source: Path instructions
🧹 Nitpick comments (2)
src/atomdb/inmemorydb/InMemoryDB.h (1)
20-27: 🚀 Performance & Scalability | 🔵 TrivialSingle recursive mutex serializes all readers and writers.
Every public method — including pure reads like
get_atom/query_for_pattern— now contends on the samerecursive_mutex. This is a reasonable, safe trade-off for reentrancy (methods call each other), but under heavy concurrent pattern-query load it removes any read/read parallelism that existed before. Worth keeping an eye on if this becomes a throughput bottleneck; ashared_mutexisn't easily reentrant-safe for the current call patterns, so this isn't a quick swap.Also applies to: 89-91
🤖 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.h` around lines 20 - 27, Keep the single internal recursive mutex and its serialization of all public InMemoryDB methods, including read operations such as get_atom and query_for_pattern; no code change is required for this advisory performance observation.src/atomdb/inmemorydb/InMemoryDB.cc (1)
462-513: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale comment: "Release locks before calling delete_atom to avoid deadlock" no longer reflects the locking model.
With the new
recursive_mutexheld vialock_guardat function entry (line 463), no lock is actually released before the recursive call intodelete_atom— the comment predates the new locking and is now misleading about how re-entrancy is achieved.✏️ Suggested comment update
- // Release locks before calling delete_atom to avoid deadlock - // Delete targets that have no other incoming links + // Safe to re-enter: api_mutex_ is recursive, so delete_atom can be called + // while this lock is still held by the current thread. + // Delete targets that have no other incoming links🤖 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 462 - 513, Update the comment immediately before the targets_to_delete loop in InMemoryDB::delete_link to accurately describe the current recursive_mutex-based locking behavior; remove the claim that locks are released before delete_atom, while preserving the existing recursive deletion logic.
🤖 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/inmemorydb/InMemoryDB.h`:
- Around line 84-85: Add brief Doxygen blocks immediately above the public
InMemoryDB methods get_all_atoms() and drop_all() in the header, documenting
each API’s purpose according to the project’s existing style.
---
Outside diff comments:
In `@src/atomdb/inmemorydb/InMemoryDB.cc`:
- Around line 190-208: Update InMemoryDB::get_matching_atoms to return deep
clones of the trie-owned atom using the existing clone_atom() helper instead of
constructing shared_ptr<Node> or shared_ptr<Link> from the raw Atom*. Preserve
the node/link result behavior while ensuring the returned pointers have
independent ownership from AtomTrieValue.
In `@src/atomdb/remotedb/RemoteAtomDB.cc`:
- Around line 415-440: Add a multi-peer batch-delete test in
remote_atomdb_test.cc covering delete_atoms, delete_nodes, and delete_links
through RemoteAtomDB with multiple replicated peers. Assert that each method
returns the maximum deletion count reported by any peer, preserving the existing
max-based semantics.
In `@src/tests/cpp/inmemorydb_test.cc`:
- Around line 546-737: Extend the InMemoryDB tests with a multithreaded stress
test that shares one database across threads and concurrently exercises
add_link, get_atom, drop_all or delete_atom, and query_for_pattern. Synchronize
thread start and completion, run enough iterations to expose races, and assert
only safe post-join invariants such as a valid atom count or empty state; ensure
the test is suitable for ThreadSanitizer and does not dereference results
invalidated by concurrent drop_all.
---
Nitpick comments:
In `@src/atomdb/inmemorydb/InMemoryDB.cc`:
- Around line 462-513: Update the comment immediately before the
targets_to_delete loop in InMemoryDB::delete_link to accurately describe the
current recursive_mutex-based locking behavior; remove the claim that locks are
released before delete_atom, while preserving the existing recursive deletion
logic.
In `@src/atomdb/inmemorydb/InMemoryDB.h`:
- Around line 20-27: Keep the single internal recursive mutex and its
serialization of all public InMemoryDB methods, including read operations such
as get_atom and query_for_pattern; no code change is required for this advisory
performance observation.
🪄 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: 793c694b-19d0-449d-9f26-da5f0f483739
📒 Files selected for processing (14)
config/das.jsonsrc/atomdb/inmemorydb/InMemoryDB.ccsrc/atomdb/inmemorydb/InMemoryDB.hsrc/atomdb/redis_mongodb/RedisMongoDB.ccsrc/atomdb/remotedb/RemoteAtomDB.ccsrc/atomdb/remotedb/RemoteAtomDB.hsrc/atomdb/remotedb/RemoteAtomDBPeer.ccsrc/atomdb/remotedb/RemoteAtomDBPeer.hsrc/tests/assets/remotedb_config_single.jsonsrc/tests/cpp/BUILDsrc/tests/cpp/inmemorydb_test.ccsrc/tests/cpp/redis_mongodb_test.ccsrc/tests/cpp/remote_atomdb_test.ccsrc/tests/main/evaluation_evolution.cc
| vector<shared_ptr<Atom>> get_all_atoms(); | ||
| void drop_all(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add brief Doxygen blocks for the new public get_all_atoms() / drop_all() APIs.
As per coding guidelines, src/**/*.h files should "Use brief Doxygen /** ... */ blocks above public API methods in C++ header files." Neither new method has one.
+ /** Returns deep clones of every stored atom. */
vector<shared_ptr<Atom>> get_all_atoms();
+ /** Removes all stored atoms and resets all indexes. */
void drop_all();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vector<shared_ptr<Atom>> get_all_atoms(); | |
| void drop_all(); | |
| /** Returns deep clones of every stored atom. */ | |
| vector<shared_ptr<Atom>> get_all_atoms(); | |
| /** Removes all stored atoms and resets all indexes. */ | |
| void drop_all(); |
🤖 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.h` around lines 84 - 85, Add brief Doxygen
blocks immediately above the public InMemoryDB methods get_all_atoms() and
drop_all() in the header, documenting each API’s purpose according to the
project’s existing style.
Sources: Coding guidelines, Path instructions
1 participant
Summary
Makes RemoteAtomDB safe under concurrent pattern queries (e.g.
run_toy.shAndNot / multiple LinkTemplates), which previously segfaulted (exit 139) due to races on the peer cache.InMemoryDB thread-safety
recursive_mutexand take it on every public API method.delete_link→delete_atom, re-index visitor →add_pattern).InMemoryDBacross threads without external locking.RemoteAtomDBPeer locking (kept simple)
InMemoryDBowns that.peer_mutex_only guards peer bookkeeping:cache_pointer swap,fetched_link_templates_, andstaged_handles_.cache()snapshot (shared_ptr); writes holdpeer_mutex_so staged handles stay consistent with the cache generation.release_cachenever drops staged writes, even on a “clear without persistence” path.Test plan
//tests/cpp:inmemorydb_test//tests/cpp:remote_atomdb_testsrc/tests/scripts/run_toy.shwith RemoteDB peers (concurrent LinkTemplates)