Deadlock Fixes

[data-coasys]

Summary

Deadlock fixes for perspective_instance.rs and prolog_service.

Commits

• wip: deadlock fix (perspective_instance.rs)
• wip: deadlock fixes, deactivate prolog subscriptions in sdnaonly mode
• fix deadlock in ensure_engine_updated() (prolog_service/mod.rs)

Created by Data at Nico's request.

Summary by CodeRabbit

• Bug Fixes

  • Reduced hangs and deadlocks by improving concurrency and lock handling across background processing and update paths.

• Performance

  • Less contention in critical sections, yielding smoother responsiveness during periodic processing and batch commits.

• Refactor

  • Internal concurrency and pool initialization reorganized for safer async interactions and clearer mode-aware processing (no public API changes).

[coderabbitai]

📝 Walkthrough

Walkthrough

Refactors lock usage to avoid deadlocks: perspective_instance.rs minimizes lock scope and moves async work outside critical sections; prolog_service/mod.rs changes engine update to perform loads and state updates while holding the write lock.

Changes

Cohort / File(s)	Summary
Perspective lock-scope refactor rust-executor/src/perspectives/perspective_instance.rs	Reworked async paths (diff_from_link_language, ensure_prolog_engine_pool, update_link, remove_link, batch/commit flows) to prefetch identifiers (uuid, owner_did, neighbourhood) before acquiring locks, defer async work until after locks, and add mode-aware early returns to avoid deadlocks.
Prolog engine update strategy rust-executor/src/prolog_service/mod.rs	Changed engine lifecycle to create/load/populate engines while holding the write lock (in-place updates), perform load error handling under locked scope, update link state synchronously, and use explicit drop() points to release locks.
Manifest changes Cargo.toml	Small manifest edits (lines added/removed) accompanying the refactors.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~70 minutes

Possibly related PRs

• Scoped Prolog engine-pools for fast inference inside a channel (ignoring other channels' data) #613: Related edits to Prolog engine pool and perspective engine initialization paths (ensure_prolog_engine_pool and pool-loading logic).
• Avoid delays in Ad4mModel.save() and batch.commit() #639: Similar lock and concurrency refactor in perspective_instance.rs affecting batch/commit and link handling.
• Fix race condition in prolog query subscriptions and resolve deadlock issues #607: Related work addressing Prolog subscription/race and deadlock issues in perspective update flows.

Suggested reviewers

• jhweir

Poem

🐰 Through tunnels of mutex and await, I hop,
I fetch UUIDs first, so locks don't stop,
I nudge engines gently, held in place,
Then drop the guard and let them race,
A tiny hare, untying deadlock knots.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Deadlock Fixes' directly and accurately summarizes the main changes in the pull request, which address deadlock issues across multiple files (perspective_instance.rs and prolog_service/mod.rs).

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch deadlock-fixes-only

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

rust-executor/src/perspectives/perspective_instance.rs (1)

1196-1213: Avoid constructing an unused LinkExpression.
The match currently builds a LinkExpression only to discard it. This extra conversion/cloning is avoidable since only the status is used.

♻️ Proposed tweak

-        let (_link, link_status) = match decorated_link_option {
-            Some(decorated) => {
-                let status = decorated.status.clone().unwrap_or(LinkStatus::Local);
-                (LinkExpression::from(decorated), status)
-            }
+        let link_status = match decorated_link_option {
+            Some(decorated) => decorated.status.clone().unwrap_or(LinkStatus::Local),
             None => {
                 return Err(AnyError::msg(format!(
                     "NH [{}] ({}) Link not found in perspective \"{}\": {:?}",
                     handle
                         .shared_url
                         .clone()
                         .unwrap_or("not-shared".to_string()),
                     handle.name.clone().unwrap_or("<no name>".to_string()),
                     handle.uuid,
                     old_link
                 )))
             }
         };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@rust-executor/src/perspectives/perspective_instance.rs` around lines 1196 -
1213, The match arm is constructing a LinkExpression via
LinkExpression::from(decorated) only to discard it; instead, avoid that
unnecessary conversion and cloning by extracting the status directly from
decorated (use decorated.status.clone().unwrap_or(LinkStatus::Local)) and return
(or assign) just the status; update the Some(decorated) arm in the
decorated_link_option match to produce only the status (remove
LinkExpression::from call) and adjust downstream usage of (_link, link_status)
or replace tuple with a single link_status variable if the unused _link can be
removed.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@rust-executor/src/perspectives/perspective_instance.rs`:
- Around line 1196-1213: The match arm is constructing a LinkExpression via
LinkExpression::from(decorated) only to discard it; instead, avoid that
unnecessary conversion and cloning by extracting the status directly from
decorated (use decorated.status.clone().unwrap_or(LinkStatus::Local)) and return
(or assign) just the status; update the Some(decorated) arm in the
decorated_link_option match to produce only the status (remove
LinkExpression::from call) and adjust downstream usage of (_link, link_status)
or replace tuple with a single link_status variable if the unused _link can be
removed.

[coderabbitai]

🧹 Nitpick comments (1)

rust-executor/src/prolog_service/mod.rs (1)

236-239: Unnecessary placeholder allocation when engine already exists.

When engine_exists is true, two PrologEngine::new() instances are created at line 238 but never inserted or used — they're silently dropped at the Entry::Occupied branch (line 282). This is wasted allocation on every dirty-update of an existing engine.

♻️ Suggested refactor: use Option to avoid unnecessary allocation

-            let (query_engine, subscription_engine) = if !engine_exists {
+            let new_engines = if !engine_exists {
                 let mut qe = PrologEngine::new();
                 qe.spawn().await?; // Expensive async operation - no lock held
 
                 let mut se = PrologEngine::new();
                 se.spawn().await?; // Expensive async operation - no lock held
 
-                (qe, se)
+                Some((qe, se))
             } else {
-                // Engines exist, we'll update them - create placeholders for now
-                (PrologEngine::new(), PrologEngine::new())
+                None
             };

Then adjust the Entry block:

             match engines.entry(engine_key.clone()) {
                 Entry::Vacant(entry) => {
-                    // Insert new engines
-                    entry.insert(SimpleEngine {
-                        query_engine,
-                        subscription_engine,
-                        dirty: true,
-                        current_links: Vec::new(),
-                        current_sdna_links: None,
-                    });
+                    if let Some((qe, se)) = new_engines {
+                        entry.insert(SimpleEngine {
+                            query_engine: qe,
+                            subscription_engine: se,
+                            dirty: true,
+                            current_links: Vec::new(),
+                            current_sdna_links: None,
+                        });
+                    } else {
+                        // engine_exists was true but entry is vacant - should not happen
+                        return Err(Error::msg("Prolog engine state inconsistency"));
+                    }
                 }
                 Entry::Occupied(_) => {
-                    // Engine was inserted concurrently; clean up the unused spawned engines
-                    // (they will be dropped automatically, no explicit stop needed)
+                    // Engine already exists (or was inserted concurrently).
+                    // Any spawned engines in new_engines will be dropped automatically.
                 }
             }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@rust-executor/src/prolog_service/mod.rs` around lines 236 - 239, The current
branch creates two PrologEngine::new() placeholders even when engine_exists is
true, wasting allocations; change the temporary pair to Option<(PrologEngine,
PrologEngine)> (or two Options) so you only construct PrologEngine instances
when needed: set the temp value to None when engine_exists is true and to
Some((PrologEngine::new(), PrologEngine::new())) when false, then update the
subsequent Entry handling (including Entry::Occupied and Entry::Vacant) to
take/create engines from the Option only when inserting new engines rather than
always allocating placeholders.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@rust-executor/src/prolog_service/mod.rs`:
- Around line 236-239: The current branch creates two PrologEngine::new()
placeholders even when engine_exists is true, wasting allocations; change the
temporary pair to Option<(PrologEngine, PrologEngine)> (or two Options) so you
only construct PrologEngine instances when needed: set the temp value to None
when engine_exists is true and to Some((PrologEngine::new(),
PrologEngine::new())) when false, then update the subsequent Entry handling
(including Entry::Occupied and Entry::Vacant) to take/create engines from the
Option only when inserting new engines rather than always allocating
placeholders.