refactor(core): replace Rc-based Context with a synchronous, Send-able Core by osmaczko · Pull Request #123 · logos-messaging/libchat

osmaczko · 2026-05-28T22:02:25Z

Make the core Send so the upcoming threaded client can own it behind an
Arc<Mutex>: a background worker polls the transport and handles
inbound payloads while the application's own thread issues outbound calls
(send, create conversation). Sharing the core across those two threads means
moving it into the spawned worker, which is only legal if it is Send. Access
stays serialized by the client's Mutex (one thread at a time), so the core
needs Send but not Sync, and carries no lock of its own. See
docs/adr/0001-client-event-system.md for the background-poller design.

Dissolve the Rc service-sharing that made the conversations core
!Send. Context is de-Rc'd and renamed Core: it owns its services outright
(identity, delivery, store, registry) and drives the inbox/conversation
primitives with plain &mut self.

Inbox, InboxV2, PrivateV1Convo, GroupV1Convo become non-generic and take
services as &mut/& parameters; no Rc or RefCell-as-shared-state remains.
CausalHistoryStore drops its Rc (keeps a Send RefCell).
The Convo/GroupConvo/Id trait objects are removed: Core dispatches on
ConversationKind and exposes group operations directly, so conversations
never escape the orchestrator.
A Send static-assert pins the invariant: the core is Send whenever its
services are, with no thread-affinity of its own.

Copilot

Pull request overview

Refactors the conversations core to remove Rc<RefCell<_>>-based service sharing, renaming Context to Session and making it own its services outright. Inbox/Conversation primitives become non-generic transient structs that take services and storage as &mut parameters per call. A compile-time assertion pins the Send invariant when injected services are Send. The trait objects Convo/GroupConvo/Id are removed; Session dispatches by persisted ConversationKind and exposes group operations directly.

Changes:

Rename Context → Session; drop Rc/RefCell-shared services and dispatch by ConversationKind in Session; add a Send static-assert.
De-generic Inbox, InboxV2, PrivateV1Convo, GroupV1Convo and remove Convo/GroupConvo/Id traits; pass identity/store/delivery/registry per call.
CausalHistoryStore keeps a RefCell but drops Rc; updated call sites in client, FFI, and integration tests.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
Cargo.toml	Workspace member list edits (formatting regression: two entries on one line).
core/conversations/src/lib.rs	Replace `context` module/exports with `session::{Session, ConversationId, Introduction}`.
core/conversations/src/session.rs	New `Session` owning services directly; per-kind dispatch; Send assertion.
core/conversations/src/conversation.rs	Remove `Id`/`Convo`/`GroupConvo` traits, keep only re-exports.
core/conversations/src/conversation/privatev1.rs	`PrivateV1Convo` becomes non-generic; store passed per-op.
core/conversations/src/conversation/group_v1.rs	`GroupV1Convo` becomes non-generic; ctx/ds/causal/keypkg passed per-op.
core/conversations/src/inbox/handler.rs	`Inbox` becomes non-generic; identity/store passed per-op.
core/conversations/src/inbox_v2.rs	`InboxV2` becomes non-generic; adds `create_group`/`group_send_content`/`group_add_member`/`handle_group_message`.
core/conversations/src/causal_history.rs	`CausalHistoryStore` drops `Rc`, keeps `RefCell`.
crates/client/src/client.rs	Use `Session` and `&mut DS` via `ctx.ds()`.
core/integration_tests_core/tests/private_integration.rs	Migrate from `Context` to `Session`.
core/integration_tests_core/tests/mls_integration.rs	Use `Session` and new direct group methods; drop `convo()` helper.
core/integration_tests_core/tests/causal_history.rs	Use `Session` and `group_send_content`.

Comments suppressed due to low confidence (1)

core/conversations/src/session.rs:53

Minor: the let mut delivery = delivery; / let mut store = store; rebindings can be avoided by declaring the parameters as mut delivery: DS and mut chat_store: CS (and mut registration: RS in new_with_name) in the function signature. Same pattern appears in new_from_store at lines 52–53 and new_with_name at lines 93–95. Purely a readability nit.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

    "core/double-ratchets",
-    "core/integration_tests_core",
-    "core/sqlite",
+    "core/integration_tests_core",    "core/sqlite",


    "core/double-ratchets",
-    "core/integration_tests_core",
-    "core/sqlite",
+    "core/integration_tests_core",    "core/sqlite",


jazzz · 2026-06-02T05:08:51Z

@osmaczko can you link to the project issue that this PR belongs to?

osmaczko · 2026-06-02T05:37:10Z

@osmaczko can you link to the project issue that this PR belongs to?

This is part of Client Event System, as per ADR's 0001:

Event is an asynchronous notification. The client's constructor returns a Receiver alongside the client handle. A background poller drives the transport, calls into the core for each inbound payload, translates the resulting PayloadOutcome into one event per observation, and pushes them onto the channel. Background work that has no synchronous trigger at all (delivery retry timeouts, future protocol timers) pushes onto the same channel.

This PR is a prerequisite for enabling the client to perform background, threaded transport polling. I split the changes into two PRs (this + followup with background processing) to make the review easier.

Do you think it makes sense to close Client Event System issue and open new one to track that? For instance: Threaded Client?

…e Core Make the core Send so the upcoming threaded client can own it behind an Arc<Mutex<Core>>: a background worker polls the transport and handles inbound payloads while the application's own thread issues outbound calls (send, create conversation). Sharing the core across those two threads means moving it into the spawned worker, which is only legal if it is Send. Access stays serialized by the client's Mutex (one thread at a time), so the core needs Send but not Sync, and carries no lock of its own. See docs/adr/0001-client-event-system.md for the background-poller design. Dissolve the Rc<RefCell> service-sharing that made the conversations core !Send. Context is de-Rc'd and renamed Core: it owns its services outright (identity, delivery, store, registry) and drives the inbox/conversation primitives with plain &mut self. - Inbox, InboxV2, PrivateV1Convo, GroupV1Convo become non-generic and take services as &mut/& parameters; no Rc or RefCell-as-shared-state remains. - CausalHistoryStore drops its Rc (keeps a Send RefCell). - The Convo/GroupConvo/Id trait objects are removed: Core dispatches on ConversationKind and exposes group operations directly, so conversations never escape the orchestrator. - A Send static-assert pins the invariant: the core is Send whenever its services are, with no thread-affinity of its own.

jazzz

In general. I like this direction.

Removing run time borrow checks is absolutely needed.
Renaming Context is a welcome change.

I'm not entirely convinced that removing dynamic dispatch is needed/helpful in the long term. But if it simplifies things now, I don't see any blockers.

jazzz · 2026-06-02T21:20:42Z

+// primitives with plain `&mut self`.
+pub struct Core<DS: DeliveryService, RS: RegistrationService, CS: ChatStore> {
+    identity: Identity,
+    ds: DS,


[!] Longterm Its uncertain if the core can be the owner of the DS. There are possible usecases where the client, or the app may require access to the DS.

Legitimate forward-looking flag 👍 If that ever becomes the case, shared ownership should be introduced. Let’s KISS for now.

shared ownership should be introduced

Shared ownership is complicated. I'd look at passing in the ProviderContext to Core by reference. That way it could be owned by client, without any runtime borrow checks

jazzz · 2026-06-02T21:26:08Z

-        let (convo_id, envelopes) = self.ctx.create_private_convo(&intro, initial_content)?;
-        self.dispatch_all(envelopes)?;
-        Ok(convo_id)
+        self.core
+            .create_private_convo(&intro, initial_content)
+            .map_err(Into::into)
    }


This is much cleaner. Love this direction.

jazzz · 2026-06-02T21:33:32Z

+    pub fn send_message(&mut self, convo_id: &str, content: &[u8]) -> Result<(), ClientError> {
+        self.core
+            .send_content(convo_id, content)
+            .map_err(Into::into)


[Dust] The map_error can be avoided, by invoking the conversion directly.

as Chat(#[from] ChatError), is defined; You can use the try operator with an explicit Ok of unit

self.core.send_content(convo_id, content)?; Ok(())

jazzz · 2026-06-02T21:44:51Z

+    pub fn group_add_member(
+        &mut self,
+        convo_id: &str,
+        members: &[&AccountId],
+    ) -> Result<(), ChatError> {
+        let mut convo = self.load_mls_convo(convo_id)?;
+        convo.add_member(members, &self.mls_ctx, &mut self.ds, &self.contact_registry)
+    }


[Sand] By removing the GroupConvoTrait, this now becomes a runtime error (rather than compile time) when account_id is not of the correct type.

Not critical for V0.2 however less than Ideal.

[?] What was the issue that led to the removal of the ConvoTraits Traits

jazzz · 2026-06-02T21:47:12Z

+            ConversationKind::PrivateV1 => self.private_send_content(record, content),
+            ConversationKind::GroupV1 => self.group_send_content(&record.local_convo_id, content),
+            ConversationKind::Unknown(_) => Err(ChatError::BadBundleValue(format!(
+                "unsupported conversation type: {}",
+                record.kind.as_str()
+            ))),


[Dust] Harder to manage over dynamic dispatch

jazzz · 2026-06-02T22:02:19Z

+{
+    fn is_send<T: Send>() {}
+    is_send::<Core<DS, RS, CS>>();
+}


[Sand] This is cute - However It doesn't seem needed.

If you want to enforce that DS, RS, CS are Send so that Send can be auto implemented on Core

Add the Send requirement to service_traits::DeliveryService. That way its enforced everywhere cleanly

Better yet. Core does not require Send to operate. It's only needed if Client requires Send. Move this constraint to the Client. Have the client enforce that all types that are passed int are Send compatible. This enforce the compile time check when building Client, and allows test cases or other use cases to use !Send Services if there is no conflict.

jazzz · 2026-06-02T22:19:57Z

+        ctx: &MlsCtx,
+        ds: &mut DS,
+        keypkg_provider: &KP,
+    ) -> Result<(), ChatError> {


Moving services to be passed in on invocation is a positive change; rather than references being retained which complicates ownership.

Specifically as this PR removes dynamic dispatch, there are no requirements for functions to be similar across Conversations of the the same class.

However

[Sand] Consider combining Services into a common provider object. There are more services required to facilitate DeMLS. Adding them all as parameters will be harder to manage at various callsites, and implementations.

Here is an example usage: group_v1::add_client

With Core mapping to core_client::InnerClient

If desirable to constrain which services a function has access to, the ExternalServices bounds can be updated per function.

osmaczko requested a review from Copilot May 28, 2026 22:02

Copilot started reviewing on behalf of osmaczko May 28, 2026 22:02 View session

Copilot AI reviewed May 28, 2026

View reviewed changes

Comment thread Cargo.toml

"core/double-ratchets",

"core/integration_tests_core",

"core/sqlite",

"core/integration_tests_core", "core/sqlite",

Comment thread Cargo.toml

"core/double-ratchets",

"core/integration_tests_core",

"core/sqlite",

"core/integration_tests_core", "core/sqlite",

osmaczko force-pushed the feat/core-session-restructure branch 3 times, most recently from 4f7a9c8 to d02d508 Compare June 1, 2026 19:05

osmaczko changed the title ~~refactor(core): replace Rc-based Context with a synchronous, Send-able Session~~ refactor(core): replace Rc-based Context with a synchronous, Send-able Core Jun 1, 2026

osmaczko force-pushed the feat/core-session-restructure branch from d02d508 to cab032a Compare June 1, 2026 19:18

osmaczko requested a review from jazzz June 1, 2026 19:18

osmaczko force-pushed the feat/core-session-restructure branch from cab032a to a803bb0 Compare June 2, 2026 06:35

jazzz reviewed Jun 2, 2026

View reviewed changes

Conversation

osmaczko commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

jazzz commented Jun 2, 2026

Uh oh!

osmaczko commented Jun 2, 2026

Uh oh!

jazzz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

osmaczko commented May 28, 2026 •

edited

Loading