Refactor: Use Separate SurrealDB Database per Perspective#636
Conversation
WalkthroughDocumentation updates introducing integration testing guidelines and Rust executor testing procedures. Core architectural refactor implementing per-perspective SurrealDB isolation by injecting a per-instance Changes
Sequence DiagramsequenceDiagram
participant PM as Perspective Manager
participant SI as Service Injector
participant SS as SurrealDBService
participant PI as PerspectiveInstance
participant BG as Background Tasks
Note over PM,BG: New Per-Perspective Isolation Flow
PM->>SI: add_perspective(handle)
SI->>SS: new("ad4m", db_name)
activate SS
SS-->>SI: SurrealDBService instance
deactivate SS
SI->>PI: new(handle, created_from_join, surreal_service)
activate PI
PI->>PI: store surreal_service field
PI-->>SI: PerspectiveInstance
deactivate PI
SI->>PM: store instance in PERSPECTIVES map
activate PM
Note over PM: Instance now accessible<br/>with injected service
deactivate PM
SI->>PI: sync_links()
activate PI
PI->>SS: query_links/add_link/remove_link
Note over SS: Per-perspective DB<br/>no global filtering
SS-->>PI: results
PI-->>SI: sync complete
deactivate PI
SI->>PI: start_background_tasks()
activate PI
PI->>BG: spawn tasks
activate BG
BG->>SS: operations use self.surreal_service
Note over BG: All tasks use<br/>injected service
deactivate PI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
rust-executor/src/perspectives/mod.rs (1)
299-319: Imported perspectives don’t populate their Surreal cacheWith the move to per-perspective Surreal DBs,
import_perspectiveonly writes links intoAd4mDbviaadd_many_links, but never syncs those into the imported perspective’s Surreal instance. Until the process is restarted (andinitialize_from_db()runssync_existing_links_to_surreal()),surreal_query/Surreal subscriptions on an imported perspective will see an empty or incomplete cache.If the expectation is that Surreal-based queries work immediately after import, consider explicitly syncing here, e.g.:
- after
add_many_links, fetch the instance viaget_perspective(&instance.handle.uuid)and callsync_existing_links_to_surreal().await, or- add a helper on
SurrealDBServiceto reload fromAd4mDbby UUID and call it here.rust-executor/src/surreal_service/mod.rs (3)
1163-1422:test_reload_perspective_data_integritystill usesWHERE perspective = $perspectiveand will now failInside this test you still build queries like:
SELECT * FROM link WHERE perspective = $perspective ORDER BY timestamp- Multiple
graph_query*strings withWHERE perspective = $perspective AND ...Since the schema no longer defines a
perspectivefield andquery_linksno longer binds$perspective, these queries will fail (unbound$perspective) rather than returning data. The intent of the test is now covered by per‑DB isolation; you can safely drop the explicit perspective filter in all these queries.- let query = "SELECT * FROM link WHERE perspective = $perspective ORDER BY timestamp"; + let query = "SELECT * FROM link ORDER BY timestamp"; @@ - let graph_query_before = "SELECT * FROM link WHERE perspective = $perspective AND count(->link[WHERE predicate = 'flux://member']) > 0"; + let graph_query_before = "SELECT * FROM link WHERE count(->link[WHERE predicate = 'flux://member']) > 0"; @@ - let graph_query1 = - "SELECT * FROM link WHERE perspective = $perspective AND in.uri = 'ad4m://self'"; + let graph_query1 = + "SELECT * FROM link WHERE in.uri = 'ad4m://self'"; @@ - let graph_query2 = "SELECT * FROM link WHERE perspective = $perspective AND out.uri = 'flux://has_channel'"; + let graph_query2 = "SELECT * FROM link WHERE out.uri = 'flux://has_channel'"; @@ - let graph_query3 = "SELECT * FROM link WHERE perspective = $perspective AND in.uri = 'ad4m://self' AND predicate = 'flux://message'"; + let graph_query3 = "SELECT * FROM link WHERE in.uri = 'ad4m://self' AND predicate = 'flux://message'"; @@ - let graph_query4 = "SELECT * FROM link WHERE perspective = $perspective AND out.uri = 'flux://has_channel' AND predicate = 'flux://entry_type'"; + let graph_query4 = "SELECT * FROM link WHERE out.uri = 'flux://has_channel' AND predicate = 'flux://entry_type'"; @@ - let graph_query5 = "SELECT * FROM link WHERE perspective = $perspective AND in.uri = 'ad4m://self' AND predicate IN ['flux://entry_type', 'rdf://name']"; + let graph_query5 = "SELECT * FROM link WHERE in.uri = 'ad4m://self' AND predicate IN ['flux://entry_type', 'rdf://name']";
1424-1578:test_reload_perspective_with_large_datasetalso relies onperspective = $perspectiveSimilar issue here: this test still uses
WHERE perspective = $perspectivein both the count query and graph traversal queries. Under the new per‑DB model, those references should be removed and the queries should just run againstlinkin the current DB.- let query = "SELECT * FROM link WHERE perspective = $perspective"; + let query = "SELECT * FROM link"; @@ - let graph_query_source = - "SELECT * FROM link WHERE perspective = $perspective AND in.uri = 'new_source://0'"; + let graph_query_source = + "SELECT * FROM link WHERE in.uri = 'new_source://0'"; @@ - let graph_query_target = - "SELECT * FROM link WHERE perspective = $perspective AND out.uri = 'new_target://0'"; + let graph_query_target = + "SELECT * FROM link WHERE out.uri = 'new_target://0'"; @@ - let graph_query_predicate = "SELECT * FROM link WHERE perspective = $perspective AND predicate = 'new_predicate://0' AND in.uri = 'new_source://0'"; + let graph_query_predicate = "SELECT * FROM link WHERE predicate = 'new_predicate://0' AND in.uri = 'new_source://0'";
391-462: Multiple queries still use$perspectivebinding, which will now failThe
query_links()method no longer binds$perspective, but at least 9 hard-coded queries inrust-executor/src/surreal_service/mod.rs(lines 1206, 1225, 1351, 1367, 1383, 1395, 1408, 1451, 1534, 1551, 1568) still containWHERE perspective = $perspective. These will fail with unbound variable errors at runtime. Additionally, the TypeScript query builder incore/src/model/Ad4mModel.tsgenerates queries with$perspective, and test assertions incore/src/model/Ad4mModel.test.tsexpect this pattern in 20+ cases.Ensure all SurrealDB queries have been updated to remove
$perspectivereferences, and verify the query builder has been modified to not inject this parameter.
🧹 Nitpick comments (9)
.claude/skills/rust-executor-testing.md (1)
15-22: Clarify whether these flag requirements are specific to this refactor or pre-existing.The document emphasizes that
--releaseand--test-threads=1flags are required but doesn't clarify whether these constraints are new due to the per-perspective isolation refactor or have always been necessary for rust-executor tests. This distinction matters for developers deciding whether to apply these flags globally or only for this PR.Consider adding a brief note on whether these flags should be applied going forward for all rust-executor tests or are temporary requirements.
.claude/skills/integration-testing.md (2)
40-49: Add cross-platform guidance for process cleanup.The cleanup script uses
killall -9, which may not be available on all platforms (particularly Windows). Consider documenting fallback commands or platform-specific alternatives to ensure the guidance is actionable for developers on all major operating systems.Example guidance:
# Linux/macOS killall -9 ad4m-executor # Windows (PowerShell) Stop-Process -Name "ad4m-executor" -Force -ErrorAction SilentlyContinue
1-54: Comprehensive and practical integration testing guide.This documentation clearly addresses a critical pre-test requirement (process cleanup) and provides excellent troubleshooting guidance for hanging tests. The root cause explanation (lines 29–36) helps developers understand why cleanup is necessary, not just that it's necessary, which should reduce future support burden.
The structured approach—problem statement, diagnosis, solution, best practices, and sample script—makes this a valuable resource for developers unfamiliar with the new per-perspective isolation architecture.
rust-executor/src/perspectives/mod.rs (2)
41-72: Initial Surreal sync can race with early API callsThe new per-perspective init flow (create
SurrealDBService, insert intoPERSPECTIVES, thensync_existing_links_to_surreal()before starting background tasks) is a big improvement, but there’s still a narrow window where callers can obtain the instance viaget_perspective()and executeadd_link/surreal_querywhilereload_perspective()is still in-flight. Ifreload_perspective()does a truncate-and-repopulate, those concurrent writes can be temporarily or permanently dropped from the Surreal cache.Consider either:
- pre-populating Surreal before inserting into
PERSPECTIVES, or- gating Surreal usage on an “initialized” flag set after the sync, or
- making
reload_perspective()merge instead of replace.
95-106: Start background tasks only after registering the instance inPERSPECTIVESIn
add_perspective,start_background_tasks()is spawned before the instance is inserted intoPERSPECTIVES. Background loops which eventually callupdate_perspective_state()go throughupdate_perspective(), which looks up the handle inPERSPECTIVES; in a bad interleaving this can log “Perspective with uuid … not found” even though creation succeeded.You can avoid this small race (and make the flow consistent with
initialize_from_db) by inserting the cloned instance first and then spawning background tasks, e.g.:- let p = PerspectiveInstance::new(handle.clone(), created_from_join, surreal_service); - tokio::spawn(p.clone().start_background_tasks()); - - { - let mut perspectives = PERSPECTIVES.write().unwrap(); - perspectives.insert(handle.uuid.clone(), RwLock::new(p)); - } + let p = PerspectiveInstance::new(handle.clone(), created_from_join, surreal_service); + { + let mut perspectives = PERSPECTIVES.write().unwrap(); + perspectives.insert(handle.uuid.clone(), RwLock::new(p.clone())); + } + tokio::spawn(p.start_background_tasks());rust-executor/src/surreal_service/mod.rs (4)
363-389:remove_linkignores perspective and deletes by in/out/predicate onlyUsing
in/outpluspredicateis a reasonable key for deletion now that each service instance targets a single perspective DB. Note thatensure_node()will create nodes even when removing a non‑existent link, which is unchanged behavior but still a bit counter‑intuitive; if you touch this later, consider avoiding node creation on delete.
504-511:clear_perspectivenow deletes all links in the DB (ignores UUID)Given each
SurrealDBServiceis now bound to a single perspective DB, havingclear_perspectiveissueDELETE FROM linkwithout using_perspective_uuidis logically correct. Just be aware this leaves orphanednoderows behind; that’s likely acceptable but could be revisited if storage growth becomes a concern.
513-533:reload_perspectivecorrectly rebuilds links but duplicates clear logicThe new implementation (delete all
linkrows then calladd_linkfor each provided link) matches the per‑DB semantics and keeps all creation logic centralized inadd_link. To avoid duplication, consider delegating toclear_perspectiveinstead of repeatingDELETE FROM link.- // Clear all links in this perspective's database - // (each perspective has its own database, so no filter needed) - self.db.query("DELETE FROM link").await?; + // Clear all links in this perspective's database + // (each perspective has its own database, so no filter needed) + self.clear_perspective(perspective_uuid).await?;
540-547: Legacy globalinit_surreal_servicedefault DB is acceptable as a stopgapInitializing a single
"ad4m"/"default"service for backwards compatibility makes sense while the rest of the codebase migrates to per‑perspective instances. It might be worth clearly markingget_surreal_serviceand this initializer as deprecated in docs/comments once all critical paths use per‑perspective services.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.claude/skills/integration-testing.md(1 hunks).claude/skills/rust-executor-testing.md(1 hunks)rust-executor/src/perspectives/mod.rs(2 hunks)rust-executor/src/perspectives/perspective_instance.rs(20 hunks)rust-executor/src/surreal_service/mod.rs(47 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: lucksus
Repo: coasys/ad4m PR: 628
File: tests/js/tests/prolog-and-literals.test.ts:2726-2729
Timestamp: 2025-09-24T14:26:44.307Z
Learning: In AD4M, perspectives have names but are uniquely identified by UUIDs. Creating a new perspective with the same name in consecutive calls will create a fresh perspective each time, so there's no risk of state leakage between tests based on perspective names.
📚 Learning: 2025-09-24T14:26:44.307Z
Learnt from: lucksus
Repo: coasys/ad4m PR: 628
File: tests/js/tests/prolog-and-literals.test.ts:2726-2729
Timestamp: 2025-09-24T14:26:44.307Z
Learning: In AD4M, perspectives have names but are uniquely identified by UUIDs. Creating a new perspective with the same name in consecutive calls will create a fresh perspective each time, so there's no risk of state leakage between tests based on perspective names.
Applied to files:
rust-executor/src/perspectives/perspective_instance.rsrust-executor/src/perspectives/mod.rsrust-executor/src/surreal_service/mod.rs
📚 Learning: 2025-09-03T15:16:29.623Z
Learnt from: lucksus
Repo: coasys/ad4m PR: 619
File: rust-executor/src/perspectives/perspective_instance.rs:639-641
Timestamp: 2025-09-03T15:16:29.623Z
Learning: In the AD4M codebase, database operations like `add_many_links` during critical synchronization processes (such as `diff_from_link_language`) should use `.expect()` rather than error logging, as failures indicate severe system issues where fail-fast behavior is preferred over continuing with potentially corrupted state.
Applied to files:
rust-executor/src/perspectives/perspective_instance.rsrust-executor/src/surreal_service/mod.rs
📚 Learning: 2025-06-05T11:39:48.641Z
Learnt from: lucksus
Repo: coasys/ad4m PR: 605
File: rust-executor/src/js_core/options.rs:59-68
Timestamp: 2025-06-05T11:39:48.641Z
Learning: In the rust-executor project, CUSTOM_DENO_SNAPSHOT.bin is a build-time generated artifact created by the generate_snapshot binary (with --features generate_snapshot). The file is not checked into source control but is created during the build process via pnpm build-deno-snapshot script and then included in subsequent builds via include_bytes! macro.
Applied to files:
.claude/skills/rust-executor-testing.md
🧬 Code graph analysis (3)
rust-executor/src/perspectives/perspective_instance.rs (1)
rust-executor/src/surreal_service/mod.rs (2)
new(246-331)init_surreal_service(540-547)
rust-executor/src/perspectives/mod.rs (2)
rust-executor/src/perspectives/perspective_instance.rs (1)
new(197-229)rust-executor/src/surreal_service/mod.rs (1)
new(246-331)
rust-executor/src/surreal_service/mod.rs (1)
rust-executor/src/perspectives/perspective_instance.rs (2)
new(197-229)db(1173-1176)
🔇 Additional comments (15)
rust-executor/src/perspectives/perspective_instance.rs (4)
169-229: Per-perspectiveSurrealDBServiceinjection is wired correctlyThe new
surreal_service: Arc<SurrealDBService>field and updatedPerspectiveInstance::new(handle, created_from_join, surreal_service)constructor cleanly inject a per-perspective service while keeping the instance constructor synchronous. CloningPerspectiveInstancenow correctly shares the same Surreal client viaArc, which is what you want for subscriptions and background loops.
247-277: Surreal sync/query/cache path is coherent and maintains invariants
sync_existing_links_to_surreal()pulls all links viaget_links(LinkQuery::default())and callssurreal_service.reload_perspective(&uuid, all_links), thensurreal_query()andupdate_surreal_cache()consistently route through the injectedsurreal_service. The “removals before additions” ordering inupdate_surreal_cache()is preserved, which is crucial for collection-style updates.Assuming
reload_perspective()is idempotent for a given(uuid, all_links)pair and only affects that DB, this setup gives each perspective a clean, isolated Surreal cache without the brittle query rewriting from before.Also applies to: 1623-1639, 1655-1673
3243-3310: Test setup and isolation test validate the new per-perspective designThe updated test
setup()/create_perspective()helpers now construct a freshSurrealDBService::new("ad4m", &uuid)perPerspectiveInstance, matching the production wiring.test_perspective_isolation_surreal_queryin particular is a nice regression test: two perspectives, two different in-memory DBs, and assertions that eachSELECT * FROM linkonly sees its own data.This should catch regressions where a shared/global Surreal instance leaks results across perspectives.
Also applies to: 3922-3971
3742-5489: Surreal query tests comprehensively cover the new behaviorThe suite of Surreal-related tests (literal parsing, structural “recipe” queries, blocking mutating operations, and the doc-style graph/aggregation examples) all target
PerspectiveInstance::surreal_queryon a per-perspective DB and no longer rely on injectedperspectivefilters. They collectively exercise:
- basic CRUD visibility between
add_link*/remove_link*and Surreal,- advanced traversal/grouping/aggregation patterns,
- the
fn::parse_literalhelper, and- enforcement that mutating queries are rejected.
Given the architectural change, this is an appropriate level of coverage and should make future regressions in Surreal query handling quite visible.
rust-executor/src/surreal_service/mod.rs (11)
97-114: Clarifying comment forLinkEdge(no more perspective column) looks goodThe updated comment correctly documents that perspective is no longer stored on
linkrecords because each perspective has its own DB. No issues with the struct layout itself.
246-331:SurrealDBService::new(namespace, database)aligns with per‑perspective DB designAccepting namespace and database explicitly and calling
use_ns(...).use_db(...)is exactly what you need for per‑perspective isolation. The schema bootstrap remains unchanged and localized to service creation.
333-361:add_linkno longer stores perspective field — consistent with per‑DB isolationKeeping
_perspective_uuidfor API compatibility while dropping any use of it insideadd_linkis consistent with the new architecture. TheRELATEstatement and explicitsource/targetfields still look correct.
585-802: Basic test updates toSurrealDBService::new("ad4m", "<db>")look correctThe early tests (
test_new_service_initializes_successfullythroughtest_nodes_are_created_for_links) now explicitly pass namespace and a DB name, which is consistent with the new constructor and per‑perspective model. Using separateSurrealDBService::newcalls per test keeps isolation.
804-924: Query‑by‑source/predicate/target tests correctly exercise graph traversal without perspective filtersThe updated tests for querying by
in.uri,out.uri, andpredicaterely solely on the link graph and implicitly on per‑DB isolation. They no longer depend on a perspective column and match the new behavior.
926-1002:test_clear_perspectivematches new semantics (DB‑scoped delete)This test now validates that
clear_perspectivewipes all links from the current DB only, which is exactly the intended behavior under per‑perspective isolation.
1004-1062:test_perspective_isolationnicely validates per‑DB separationCreating two services with different DB names and asserting that clearing one does not affect the other is a good sanity test for the new architecture.
1064-1161: Reload tests (test_reload_perspective,test_reload_perspective_with_empty_list) align with per‑DB modelBoth tests now operate entirely within a single service/DB and verify that
reload_perspectivebehaves as full replacement. They don’t rely on perspective columns and look correct.
1580-1599:test_global_service_initializationnow just sanity‑checks a non‑perspective‑bound serviceUsing
SurrealDBService::new("ad4m", "test_global_init")and confirming it can add a link is a reasonable minimal test of a non‑per‑perspective instance, consistent with the legacy global initializer.
1601-1715: Teststest_query_without_perspective_binding,test_automatic_perspective_filtering, andtest_concurrent_operationscorrectly exercise the new model
test_query_without_perspective_bindingnow proves queries don’t need explicit perspective clauses.test_automatic_perspective_filteringnicely validates that identical link patterns in separate DBs don’t leak.test_concurrent_operationsstresses concurrentadd_linkcalls; this still looks fine with the Arc‑wrapped client.All align with the per‑DB design.
1717-1930: Read‑only query validation tests comprehensively cover the new guardrailAll the
test_query_validation_*tests use the updated constructor and successfully exercise the read‑only enforcement logic (allowing SELECT/RETURN and blocking INSERT/UPDATE/DELETE/etc., including case and semicolon variants). The changes here are just constructor updates and are consistent.
2 participants
Problem
When using multiple perspectives (Neighbourhoods in Flux), one perspective would consistently return empty SurrealDB query results. The issue was non-deterministic - after an AD4M restart, a different perspective might be affected, but the problem persisted throughout runtime.
Root Cause: The complex string manipulation logic in
query_links()(200+ lines) that attempted to injectperspective = $perspectivefilters into arbitrary SurrealQL queries had subtle parsing errors. This fragile code tried to handle:FROM linkoccurrencesFROM link AS l)The parsing would fail silently for certain query patterns, resulting in incorrect or missing filters.
Solution
Complete architectural change to per-perspective database isolation.
Each
PerspectiveInstancenow gets its own isolated SurrealDB database:ad4m{perspective_uuid}This eliminates the need for any perspective filtering in queries.
Changes
Core Architecture
src/surreal_service/mod.rs:SurrealDBService::new()to takenamespaceanddatabaseparametersperspectivefield fromLinkEdgestruct and database schemaquery_links()add_link(),remove_link(),clear_perspective(), andreload_perspective()- no filtering neededsrc/perspectives/perspective_instance.rs:surreal_service: Arc<SurrealDBService>field toPerspectiveInstancenew()to takeSurrealDBServiceas parameter (created by caller in async context)src/perspectives/mod.rs:initialize_from_db(): Creates per-perspective service in spawned async taskadd_perspective(): Creates service before instantiating perspectiveTest Updates
perspectivefieldSurrealDBServiceimport to test modulesBenefits
Documentation
Added Claude Code skill files for future development:
Summary by CodeRabbit
Documentation
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.