weaverd: narrow BackendManager lock duration to exclude blocking I/O and LSP enrichment

[coderabbitai]

Background

BackendManager::with_backends acquires the Arc<Mutex<FusionBackends<SemanticBackendProvider>>> and holds it for the full duration of handler.rs line 91:

.with_backends(|backends| self.router.route(&request, &mut writer, backends));

For the observe graph-slice handler this means the lock is held across:

• Filesystem reads (read_slice_source)
• Up to 256 tree-sitter extraction calls (discover_same_file_cards)
• LSP network calls for semantic enrichment (enrich_card_if_requested)

This prevents concurrent request handling and risks lock poisoning if any of these blocking operations panics.

Task

• Narrow the lock duration so it is held only while accessing shared mutable backend state, not across blocking I/O or network operations.
• Identify whether tree-sitter extraction and LSP enrichment can be performed outside the lock by cloning or arc-ing the relevant client handles.
• Add a concurrent-request integration test that submits two observe graph-slice requests simultaneously and asserts both complete within an acceptable timeout.
• Add timeout handling to with_backends (or its callers) to surface lock contention rather than hanging indefinitely.

Context

Identified as out of scope during review of PR #106.

Raised by @leynos.

[leynos]

@CodeRabbit plan

[coderabbitai]

Coding Plan

Summary

• Arc-wrap the backend provider inside FusionBackends to allow cloning handles out from under the lock
• Refactor the graph-slice handler to acquire the lock briefly for ensure_started and handle cloning, then release it before blocking I/O and LSP calls
• Adopt parking_lot::Mutex for timeout support on lock acquisition
• Validate the fix with a concurrent-request integration test that exercises parallel graph-slice handling

Design Choices

Design Choice 1: Should the provider be Arc-wrapped within FusionBackends, or should BackendManager switch to RwLock?

Options Considered:

1. Arc-wrap provider inside FusionBackends, allowing clone-out of provider handle
2. Replace Mutex with RwLock in BackendManager, change all handler signatures to accept &FusionBackends for read operations

Chosen Option: 1

Rationale: Arc-wrapping is a localised change to FusionBackends that enables cloning handles out without changing handler signatures across the router chain.

Design Choice 2: Should timeout use parking_lot crate or a manual wrapper?

Options Considered:

1. Switch to parking_lot::Mutex which provides try_lock_for
2. Implement a spawn-thread timeout wrapper around std::sync::Mutex
3. Use std::sync::Mutex::try_lock in a polling loop with sleep

Chosen Option: 1

Rationale: parking_lot is a well-established crate with better performance characteristics and native timeout support; it is a drop-in replacement for std::sync::Mutex.

💡 User Tips

Regenerate the plan with different choices with @coderabbitai <feedback>.

Implementation Steps

Phase 1: Make Backend Handles Cloneable

Enable the provider and extractor to be cloned out from under the lock so that blocking operations can proceed without holding the BackendManager mutex.

Task 1: Arc-wrap Provider in FusionBackends

Change FusionBackends<P> to hold the provider behind an Arc, allowing cheap cloning of the handle.

• Update crates/weaverd/src/backends.rs: change provider: P to provider: Arc<P>
• Add pub fn provider_arc(&self) -> Arc<P> method returning a clone of the Arc
• Adjust FusionBackends::new() to wrap the provider in Arc on construction
• Update provider() to dereference the Arc (returns &P as before)
• Remove provider_mut() if unused, or have it return Arc::get_mut with appropriate error handling

Task 2: Update SemanticBackendProvider Construction Sites

Ensure all places that construct FusionBackends pass the provider correctly for Arc wrapping.

• Update crates/weaverd/src/bootstrap.rs where FusionBackends is created
• Update crates/weaverd/src/dispatch/handler/tests_helpers.rs fixture construction
• Update any test worlds in crates/weaverd/src/tests/support/ that build FusionBackends

🤖 Prompt for AI agents

Implement the Arc-wrapping of the backend provider inside `FusionBackends<P>` so
that provider handles can be cheaply cloned out from under the lock, enabling
lock-free access to blocking operations.

**`crates/weaverd/src/backends.rs`**
- Change the `provider` field from type `P` to `Arc<P>`
- Update `FusionBackends::new()` to wrap the incoming provider in
`Arc::new(...)` on construction
- Update `provider()` to dereference the Arc, preserving the existing `&P`
return type
- Add a new `pub fn provider_arc(&self) -> Arc<P>` method that returns
`Arc::clone(&self.provider)`
- Remove `provider_mut()` if it is unused; if it is used, replace it with
`Arc::get_mut` and handle the `None` case appropriately

**Update construction sites** to pass the provider correctly now that
`FusionBackends::new()` wraps it internally:
- `crates/weaverd/src/bootstrap.rs`
- `crates/weaverd/src/dispatch/handler/tests_helpers.rs`
- Any test support worlds under `crates/weaverd/src/tests/support/` that build
`FusionBackends`

Phase 2: Refactor Handlers for Narrow Lock Scope

Restructure the graph-slice handler and enrichment logic to acquire the lock briefly for mutable operations, then release it before blocking I/O and LSP network calls.

Task 1: Refactor enrich.rs to Accept Provider Arc Directly

Change enrichment functions to work with a cloned provider handle rather than requiring the full FusionBackends reference.

• Update crates/weaverd/src/dispatch/observe/enrich.rs: change try_lsp_enrichment signature from backends: &mut FusionBackends<SemanticBackendProvider> to provider: &SemanticBackendProvider
• Remove the backends.ensure_started(BackendKind::Semantic) call from try_lsp_enrichment (caller will handle this)
• Update initialize_and_get_capabilities and get_hover to take &SemanticBackendProvider directly
• Update enrich_card_if_requested in crates/weaverd/src/dispatch/observe/graph_slice.rs to match the new signature

Task 2: Refactor graph_slice Handler to Minimise Lock Duration

Split the handler into a preparation phase (locked) and an execution phase (unlocked).

• Update crates/weaverd/src/dispatch/observe/graph_slice.rs handle function
• Preparation phase (under lock): call backends.ensure_started(BackendKind::Semantic), clone extractor via backends.provider().card_extractor().clone(), clone provider via backends.provider_arc()
• Execution phase (no lock): perform read_slice_source, discover_same_file_cards, and enrich_card_if_requested using cloned handles
• Update discover_same_file_cards to accept the cloned extractor directly rather than extracting it internally
• Update build_response signature to accept cloned handles instead of &mut FusionBackends

Task 3: Update Router and Handler Call Sites

Adjust the router and handler to support the new lock-minimised pattern for graph-slice.

• In crates/weaverd/src/dispatch/router.rs, the route_observe match arm for "graph-slice" may need adjustment if the handler signature changes
• Consider whether other observe handlers (get_definition, get_card) benefit from similar refactoring; if so, apply the same pattern; otherwise leave them as-is for now
• Ensure crates/weaverd/src/dispatch/handler/mod.rs route_request continues to work with the refactored graph-slice handler

🤖 Prompt for AI agents

Refactor the graph-slice handler and enrichment logic to hold the
`BackendManager` mutex only for the brief mutable preparation phase, releasing
it before any blocking I/O or LSP network calls.

**`crates/weaverd/src/dispatch/observe/enrich.rs`**
- Change `try_lsp_enrichment` to accept `provider: &SemanticBackendProvider`
instead of `backends: &mut FusionBackends<SemanticBackendProvider>`
- Remove the `backends.ensure_started(BackendKind::Semantic)` call from
`try_lsp_enrichment`; the caller is now responsible for calling `ensure_started`
before passing the provider
- Update `initialize_and_get_capabilities` and `get_hover` to take
`&SemanticBackendProvider` directly

**`crates/weaverd/src/dispatch/observe/graph_slice.rs`**
- Split the `handle` function into two phases:
- **Preparation phase (under lock):** call
`backends.ensure_started(BackendKind::Semantic)`, clone the extractor via
`backends.provider().card_extractor().clone()`, and clone the provider handle
via `backends.provider_arc()`
- **Execution phase (no lock):** perform `read_slice_source`,
`discover_same_file_cards`, and `enrich_card_if_requested` using only the cloned
handles — the lock must not be held during this phase
- Update `discover_same_file_cards` to accept the cloned extractor directly
rather than extracting it internally
- Update `build_response` to accept cloned handles instead of `&mut
FusionBackends`
- Update `enrich_card_if_requested` to match the new `try_lsp_enrichment`
signature

**`crates/weaverd/src/dispatch/router.rs`**
- Adjust the `route_observe` match arm for `"graph-slice"` if the handler
signature has changed
- Evaluate other observe handlers (`get_definition`, `get_card`); apply the same
pattern if they benefit from it, otherwise leave them unchanged

**`crates/weaverd/src/dispatch/handler/mod.rs`**
- Verify `route_request` continues to compile and function correctly with the
refactored graph-slice handler

Phase 3: Add Timeout Handling and Concurrent Integration Test

Introduce timeout semantics to surface lock contention and verify the fix with a concurrent-request test.

Task 1: Switch BackendManager to parking_lot Mutex with Timeout

Replace std::sync::Mutex with parking_lot::Mutex to gain timeout support.

• Add parking_lot to crates/weaverd/Cargo.toml dependencies
• Update crates/weaverd/src/dispatch/backend_manager.rs: change Arc<Mutex<...>> to use parking_lot::Mutex
• Add with_backends_timeout method using try_lock_for(Duration) returning Result<R, DispatchError> with a timeout error variant
• Update callers to use timeout variant where appropriate, or set a sensible default timeout (e.g., 30 seconds) in the existing with_backends method
• Add DispatchError::LockTimeout variant in crates/weaverd/src/dispatch/errors.rs if one does not exist

Task 2: Add Concurrent-Request Integration Test

Create a test that submits two graph-slice requests simultaneously and asserts both complete.

• Add new test in crates/weaverd/src/dispatch/handler/tests.rs or a dedicated concurrent test module
• Use the existing HandlerTestHarness pattern but spawn two client threads
• Each thread sends an observe graph-slice request for a test fixture file
• Use a barrier or countdown latch to ensure both requests are in-flight concurrently
• Assert both responses are received within an acceptable timeout (e.g., 5 seconds)
• Verify response payloads indicate success (not lock-timeout errors)

Task 3: Add Test Fixture for Concurrent Graph-Slice Requests

Provide a suitable source file for the concurrent test to operate on.

• Add a test fixture file in crates/weaverd/src/tests/fixtures/ (or appropriate test resources directory)
• File should be small enough for fast extraction but large enough to exercise the code path
• Ensure the fixture is referenced by the concurrent test using a file:// URI

🤖 Prompt for AI agents

Introduce timeout semantics on the `BackendManager` lock using
`parking_lot::Mutex`, add a `DispatchError::LockTimeout` variant, and validate
the narrowed lock scope with a concurrent-request integration test.

**`crates/weaverd/Cargo.toml`**
- Add `parking_lot` as a dependency

**`crates/weaverd/src/dispatch/backend_manager.rs`**
- Replace `std::sync::Mutex` with `parking_lot::Mutex` for the
`Arc<Mutex<FusionBackends<...>>>` field
- Add a `with_backends_timeout` method that calls `try_lock_for(Duration)` and
returns `Result<R, DispatchError>`; return `DispatchError::LockTimeout` if the
lock is not acquired within the duration
- Update the existing `with_backends` method to use a sensible default timeout
(e.g., 30 seconds) or delegate to `with_backends_timeout`

**`crates/weaverd/src/dispatch/errors.rs`**
- Add a `LockTimeout` variant to `DispatchError` if one does not already exist

**`crates/weaverd/src/dispatch/handler/tests.rs`** (or a dedicated concurrent
test module)
- Add a concurrent integration test following the existing `HandlerTestHarness`
pattern
- Spawn two client threads; use a barrier or countdown latch to ensure both
`observe graph-slice` requests are in-flight at the same time
- Assert both responses are received within 5 seconds and that the payloads
indicate success (not `LockTimeout` errors)

**Test fixture**
- Add a small but non-trivial source file under
`crates/weaverd/src/tests/fixtures/` (or the appropriate test resources
directory)
- Reference it in the concurrent test via a `file://` URI

Research

The weaverd crate's dispatch system routes requests through DispatchConnectionHandler, which acquires an Arc<Mutex<FusionBackends<SemanticBackendProvider>>> via BackendManager::with_backends. Currently, the lock is held for the entire duration of router.route(), including blocking operations. TreeSitterCardExtractor is Clone with Arc-wrapped internals. SemanticBackendProvider uses interior mutability via Mutex<Option<LspHost>> and its LSP methods take &self. The only operation requiring &mut FusionBackends is ensure_started; all subsequent LSP calls go through backends.provider() (a shared &P reference).

---

🚀 Next Steps

🤖 All AI agent prompts combined

Task: 1

Implement the Arc-wrapping of the backend provider inside `FusionBackends<P>` so
that provider handles can be cheaply cloned out from under the lock, enabling
lock-free access to blocking operations.

**`crates/weaverd/src/backends.rs`**
- Change the `provider` field from type `P` to `Arc<P>`
- Update `FusionBackends::new()` to wrap the incoming provider in
`Arc::new(...)` on construction
- Update `provider()` to dereference the Arc, preserving the existing `&P`
return type
- Add a new `pub fn provider_arc(&self) -> Arc<P>` method that returns
`Arc::clone(&self.provider)`
- Remove `provider_mut()` if it is unused; if it is used, replace it with
`Arc::get_mut` and handle the `None` case appropriately

**Update construction sites** to pass the provider correctly now that
`FusionBackends::new()` wraps it internally:
- `crates/weaverd/src/bootstrap.rs`
- `crates/weaverd/src/dispatch/handler/tests_helpers.rs`
- Any test support worlds under `crates/weaverd/src/tests/support/` that build
`FusionBackends`
===============================================================================

Task: 2

Refactor the graph-slice handler and enrichment logic to hold the
`BackendManager` mutex only for the brief mutable preparation phase, releasing
it before any blocking I/O or LSP network calls.

**`crates/weaverd/src/dispatch/observe/enrich.rs`**
- Change `try_lsp_enrichment` to accept `provider: &SemanticBackendProvider`
instead of `backends: &mut FusionBackends<SemanticBackendProvider>`
- Remove the `backends.ensure_started(BackendKind::Semantic)` call from
`try_lsp_enrichment`; the caller is now responsible for calling `ensure_started`
before passing the provider
- Update `initialize_and_get_capabilities` and `get_hover` to take
`&SemanticBackendProvider` directly

**`crates/weaverd/src/dispatch/observe/graph_slice.rs`**
- Split the `handle` function into two phases:
- **Preparation phase (under lock):** call
`backends.ensure_started(BackendKind::Semantic)`, clone the extractor via
`backends.provider().card_extractor().clone()`, and clone the provider handle
via `backends.provider_arc()`
- **Execution phase (no lock):** perform `read_slice_source`,
`discover_same_file_cards`, and `enrich_card_if_requested` using only the cloned
handles — the lock must not be held during this phase
- Update `discover_same_file_cards` to accept the cloned extractor directly
rather than extracting it internally
- Update `build_response` to accept cloned handles instead of `&mut
FusionBackends`
- Update `enrich_card_if_requested` to match the new `try_lsp_enrichment`
signature

**`crates/weaverd/src/dispatch/router.rs`**
- Adjust the `route_observe` match arm for `"graph-slice"` if the handler
signature has changed
- Evaluate other observe handlers (`get_definition`, `get_card`); apply the same
pattern if they benefit from it, otherwise leave them unchanged

**`crates/weaverd/src/dispatch/handler/mod.rs`**
- Verify `route_request` continues to compile and function correctly with the
refactored graph-slice handler
===============================================================================

Task: 3

Introduce timeout semantics on the `BackendManager` lock using
`parking_lot::Mutex`, add a `DispatchError::LockTimeout` variant, and validate
the narrowed lock scope with a concurrent-request integration test.

**`crates/weaverd/Cargo.toml`**
- Add `parking_lot` as a dependency

**`crates/weaverd/src/dispatch/backend_manager.rs`**
- Replace `std::sync::Mutex` with `parking_lot::Mutex` for the
`Arc<Mutex<FusionBackends<...>>>` field
- Add a `with_backends_timeout` method that calls `try_lock_for(Duration)` and
returns `Result<R, DispatchError>`; return `DispatchError::LockTimeout` if the
lock is not acquired within the duration
- Update the existing `with_backends` method to use a sensible default timeout
(e.g., 30 seconds) or delegate to `with_backends_timeout`

**`crates/weaverd/src/dispatch/errors.rs`**
- Add a `LockTimeout` variant to `DispatchError` if one does not already exist

**`crates/weaverd/src/dispatch/handler/tests.rs`** (or a dedicated concurrent
test module)
- Add a concurrent integration test following the existing `HandlerTestHarness`
pattern
- Spawn two client threads; use a barrier or countdown latch to ensure both
`observe graph-slice` requests are in-flight at the same time
- Assert both responses are received within 5 seconds and that the payloads
indicate success (not `LockTimeout` errors)

**Test fixture**
- Add a small but non-trivial source file under
`crates/weaverd/src/tests/fixtures/` (or the appropriate test resources
directory)
- Reference it in the concurrent test via a `file://` URI

💡 Iterate on the plan with: `@coderabbitai` <feedback>

Example Feedback
- `@coderabbitai` You can skip phase 3. Add a simple unit test case for phase 2.
- `@coderabbitai` For design choice 1 go ahead with option 3 and replan.

---

💬 Have feedback or questions? Drop into our discord!