Skip to content

Enforce single-owner contract on InMemoryPersister#1534

Open
DanGould wants to merge 1 commit intopayjoin:masterfrom
DanGould:inmemory-persister-single-owner
Open

Enforce single-owner contract on InMemoryPersister#1534
DanGould wants to merge 1 commit intopayjoin:masterfrom
DanGould:inmemory-persister-single-owner

Conversation

@DanGould
Copy link
Copy Markdown
Contributor

@DanGould DanGould commented May 7, 2026

Follow-up to #1528 addressing review feedback from @nothingmuch in
#1528 (comment).

more net deletes 🤗

Summary

The previous InMemoryPersister (and InMemoryAsyncPersister) was Clone and used Arc<RwLock<InnerStorage<V>>> internally. That shape invites the concurrent-write footgun nothingmuch flagged: a caller can clone the persister and share write access across actors, which is a logic bug against the SessionPersister contract — sessions are conceptually single-actor.

This PR makes the single-owner intent type-level enforced:

  • Drop #[derive(Clone)] from both InMemoryPersister and InMemoryAsyncPersister. Callers that need shared access wrap in Arc<InMemoryPersister<V>>.
  • Collapse Arc<RwLock<InnerStorage<V>>>Mutex<InnerStorage<V>> (sync uses std::sync::Mutex, async uses tokio::sync::Mutex). The inner Arc only existed to make Clone cheap; without Clone it's dead weight. RwLock semantics never mattered here — both reads and writes serialize through the lock.
  • Simplify InnerStorage<V>::events from Arc<Vec<V>> to plain Vec<V>. The Arc::make_mut + try_unwrap pattern always fell through to deep clone in practice (≥2 Arc holders at load time), so the indirection added zero value.
  • Tighten the doc comment to state the single-owner contract.

In-scope side effects (mechanical, no behavior change):

  • inner.read()/inner.write()inner.lock() at every pub(crate) access site across payjoin/src/core/receive/v2/{mod,session}.rs, payjoin/src/core/send/v2/session.rs, and persist test helpers.
  • Dropped a dead + Clone trait bound on the do_v2_to_v2<R, S> test helper in integration.rs — the function body only takes &persister, never clones.

No callers in-tree clone an InMemoryPersister (the only .clone()s in payjoin-ffi are on Arc<dyn JsonReceiverSessionPersister> — unrelated). Compatible with #1533, which already wraps InMemoryPersister<String> in newtype Arc<Self> (single producer, sharing via Arc<Newtype> not Arc<InMemoryPersister>) — same architectural intent.

Disclosure: co-authored by claude-opus-4-7-1m

PR payjoin#1528 review feedback flagged that `Arc<RwLock<InnerStorage>>`
inside `Clone`-able `InMemoryPersister` invites concurrent-write
footgun: cloning the persister and sharing write access across
actors is a logic bug against the SessionPersister contract.

Drop `#[derive(Clone)]` and collapse internals to `Mutex<...>`.
Callers that genuinely need sharing must opt in via
`Arc<InMemoryPersister<V>>` and own the locking discipline.

Same change applies to `InMemoryAsyncPersister` (still cfg(test)).
@coveralls
Copy link
Copy Markdown
Collaborator

Coverage Report for CI Build 25483072556

Coverage decreased (-0.01%) to 85.156%

Details

  • Coverage decreased (-0.01%) from the base build.
  • Patch coverage: 32 of 32 lines across 4 files are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 13507
Covered Lines: 11502
Line Coverage: 85.16%
Coverage Strength: 400.3 hits per line

💛 - Coveralls

@DanGould DanGould requested a review from spacebear21 May 7, 2026 12:17
@@ -838,40 +835,33 @@ where
type SessionEvent = V;

fn save_event(&self, event: Self::SessionEvent) -> Result<(), Self::InternalStorageError> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fn save_event(&self, event: Self::SessionEvent) -> Result<(), Self::InternalStorageError> {
fn save_event(&mut self, event: Self::SessionEvent) -> Result<(), Self::InternalStorageError> {

maybe it makes sense to change the trait definition... @arminsabouri ? thoughts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested fn save(&mut self, …) on JsonReceiverSessionPersister with clankers. It fails to compile:
E0596: cannot borrow data in an Arc as mutable, both in the uniffi::export(with_foreign) macro
expansion and CallbackPersisterAdapter::save_event. Arc<dyn Trait> only implements
Deref, not DerefMut — not uniffi-specific.

Workarounds (split native vs FFI traits , or Arc<Mutex<dyn Trait>> callbacks) just
relocate the lock rather than remove it. Curious what @arminsabouri thinks — for this
PR I'd like to land the footgun fix and treat trait shape as its own discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants