Skip to content

Close out #736: convergence replay reuse/bound (#635) + daemon lock scoping (#633)#770

Merged
erskingardner merged 3 commits into
masterfrom
claude/wizardly-hellman-097779
Jul 2, 2026
Merged

Close out #736: convergence replay reuse/bound (#635) + daemon lock scoping (#633)#770
erskingardner merged 3 commits into
masterfrom
claude/wizardly-hellman-097779

Conversation

@erskingardner

@erskingardner erskingardner commented Jul 1, 2026

Copy link
Copy Markdown
Member

Closes out the remaining open children of tracking issue marmot-protocol/mdk#380. The bulk of marmot-protocol/mdk#380 landed in the merged #768 (five boundary contracts); the only children left open were the two deliberately-deferred items (#635, marmot-protocol/mdk#438) plus one stale bug (#692). This PR resolves all three.

marmot-protocol/mdk#437 — convergence BFS replay: reuse + bound (safe subset)

The deferred full incremental-prefix rework stays deferred (branch-selection risk). This is the safe subset onlyzero change to which branch wins.

  • Reuse. The candidate-path BFS already fully materializes each completed path at its terminal probe, but canonicalize_openmls_batch re-materialized every path a second time. The terminal OpenMlsMaterializedCandidate is now retained on the completed path and, on the stored-message hot path, fed straight into a new private canonicalize_openmls_batch_with_materialized core instead of replaying again.
    • Conditional by design. The BFS probe folds pending proposals but not applications, so with pending app messages the reused candidate would miss the ApplicationProcessed observations app_messages_by_id consumes. Reuse therefore fires only when the pass has no pending application messages (byte-identical by construction); otherwise the existing fresh re-materialize runs. The public canonicalize_openmls_batch keeps its exact prior behavior (used directly by conformance vectors).
  • Bound. A per-pass ReplayBudget (derived from commit count × max_rewind_commits) caps total OpenMLS replay round-trips and fails closed (ReplayBudgetExceeded) rather than returning a partial result, bounding attacker-driven same-epoch branching. Generous enough that the default (max_rewind_commits = 5) never trips.

Tests: engine_reuses_bfs_materialized_candidates_when_no_pending_app_messages drives an app-free multi-commit chain through the reuse branch; inline replay_budget_tests cover the budget arithmetic (incl. saturation) and fail-closed exhaustion. Full cgka-engine (39 convergence) + cgka-conformance-simulator (73) suites green.

marmot-protocol/mdk#438 — daemon workers lock held across relay I/O (head-of-line blocking)

The Execute/StreamWatch arms held the shared workers async mutex across the entire command (including relay I/O and run_cli_local), so one slow relay-backed command stalled every other command and the subscription reconcile. Now the lock is held only for the host-mutating reconcile (mirroring the already-correct subscription handlers), and the relay work runs off the lock:

  • reconcile_and_clone_runtime: lock → reconcile → clone the runtime handle → release.
  • Execute-path handlers (account setup, hosted command dispatch, refresh) reconcile under the lock, then do relay I/O against the cloned runtime handle unlocked.
  • run_cli_local (opens its own account/session; touches no shared state) runs fully off the lock — the core head-of-line fix.
  • StreamWatch reconciles under the lock, then spawns the watch + opens the broker connection unlocked (the stream-watch registry is already interior-mutable).
  • handle_app_runtime_command_request split into a locked Execute entry and a host-based entry for the compose path (which already holds the lock), sharing one unlocked dispatch_hosted_runtime_commandno re-entrant lock / deadlock.
  • Stream-compose (mutates the &mut session map + host together) keeps the lock for its low-traffic QUIC-preview path.

Test: daemon_execute_local_command_runs_without_holding_worker_lock holds the workers lock for the whole Execute and asserts a relay-less local command still completes. Full darkmatter-cli suite (133) green.

Honest caveat: a true slow-relay end-to-end demonstration needs a stalling relay; the unit test proves the local path never touches the lock, backed by the existing tests/cli.rs daemon integration suite. marmot-app has no counting mock TransportAdapter, so the off-lock reconcile granularity relies on by-construction correctness + the passing integration suites (same limitation noted in #768).

marmot-protocol/mdk#416 — closed as stale (no code)

The filed O(group-history) rebuild-on-re-delivery no longer reproduces on master — the !app_event_exists_tx gate is gone; a re-delivered kind-9 CHAT takes the incremental upsert path. Closed with revalidation evidence.

Scope

Closes marmot-protocol/mdk#437
Closes marmot-protocol/mdk#438

After merge, marmot-protocol/mdk#380 closes on its full in-scope set (#768 + this PR); marmot-protocol/mdk#416 is already closed as stale.

Verification

just fast-ci green (fmt + check + clippy, incl. OTLP feature builds). cargo test -p cgka-engine -p darkmatter-cli -p cgka-conformance-simulator all green.

🤖 Generated with Claude Code


Open in Stage

Summary by CodeRabbit

  • New Features
    • Stored convergence can reuse BFS materialized candidate paths when no pending application messages are present, improving convergence speed.
  • Bug Fixes
    • Added a bounded replay budget during OpenMLS projection; replay exhaustion now fails closed with a clear ReplayBudgetExceeded error.
    • Improved canonicalization reuse safety by falling back to fresh materialization when reuse isn’t valid.
    • Reduced worker-lock contention in daemon command handling, preventing local execution deadlocks.
  • Tests
    • Added coverage for replay budget scaling/exhaustion and BFS materialized-candidate reuse, plus a lock-holding daemon regression test.

erskingardner and others added 2 commits July 1, 2026 19:01
…nce replay (#635)

Part of the #736 campaign — the safe subset of the deferred convergence BFS work
(no incremental-prefix cache, so zero change to which branch wins).

- Reuse: the BFS already fully materializes each completed candidate path at its
  terminal probe, but canonicalize_openmls_batch re-materialized every path a second
  time. Retain the terminal OpenMlsMaterializedCandidate on the completed path and,
  on the stored-message hot path, feed it straight into a new private
  canonicalize_openmls_batch_with_materialized core instead of replaying again.

  Reuse is CONDITIONAL: the BFS probe folds pending proposals but NOT applications,
  so with pending app messages the reused candidate would miss the ApplicationProcessed
  observations app_messages_by_id consumes. Reuse therefore fires only when the pass
  has no pending application messages (byte-identical by construction); otherwise the
  existing fresh re-materialize runs. The public canonicalize_openmls_batch keeps its
  exact prior behavior (used directly by conformance vectors).

- Bound: a per-pass ReplayBudget (derived from commit count and max_rewind_commits)
  caps total OpenMLS replay round-trips and fails closed (ReplayBudgetExceeded) rather
  than returning a partial result, bounding attacker-driven same-epoch branching.

Tests: engine_reuses_bfs_materialized_candidates_when_no_pending_app_messages drives
an app-free multi-commit chain through the reuse branch; inline replay_budget_tests
cover the budget arithmetic (incl. saturation) and fail-closed exhaustion. Full
cgka-engine + conformance suites green.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Part of the #736 campaign — the highest-blast-radius deferred item (CLI daemon
concurrency model).

The Execute and StreamWatch arms held the shared `workers` async mutex across the
entire command, including relay network I/O and run_cli_local, so one slow relay-backed
command stalled every other command and the subscription reconcile. Now the lock is
held only for the host-mutating reconcile (mirroring the subscription handlers) and the
relay work runs off the lock:

- Add reconcile_and_clone_runtime: lock, reconcile, clone the runtime handle, release.
- Execute-path handlers (account setup, hosted command dispatch, refresh) reconcile
  under the lock, then perform relay I/O against the cloned runtime handle unlocked.
- run_cli_local (opens its own account/session; touches no shared state) runs fully
  off the lock — the core head-of-line fix.
- StreamWatch reconciles under the lock, then spawns the watch + opens the broker
  connection unlocked (the stream-watch registry is already interior-mutable).
- Split handle_app_runtime_command_request into a locked Execute entry and a host-based
  entry for the compose path (which already holds the lock), sharing one unlocked
  dispatch_hosted_runtime_command — no re-entrant lock / deadlock.
- Stream-compose (mutates the &mut session map + host together) keeps the lock for its
  low-traffic QUIC-preview path.

Test: daemon_execute_local_command_runs_without_holding_worker_lock holds the workers
lock for the whole Execute and asserts a relay-less local command still completes.
Full darkmatter-cli suite green.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 43b2cd44-e2d8-4710-b1ee-df1e0569aad5

📥 Commits

Reviewing files that changed from the base of the PR and between df9a1d0 and ea11b5e.

📒 Files selected for processing (1)
  • crates/cli/src/daemon/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/cli/src/daemon/mod.rs

Walkthrough

This PR bounds OpenMLS convergence replay with a fail-closed budget, reuses BFS-materialized candidates during canonicalization, and refactors daemon Execute/StreamWatch handling to reduce workers-lock hold time across relay I/O.

Changes

Bounded Replay Budget and BFS Candidate Reuse

Layer / File(s) Summary
ReplayBudget type and error variant
crates/cgka-engine/src/openmls_projection.rs
Defines ReplayBudget with scaling constructors and consume(), and adds ReplayBudgetExceeded with Display support.
Budget-aware materialization
crates/cgka-engine/src/openmls_projection.rs
Routes candidate-path materialization through a budgeted helper that consumes replay budget per path.
BFS probing with budget propagation
crates/cgka-engine/src/openmls_projection.rs
Threads replay budget through BFS probing, stores materialized candidates on probe nodes, and initializes the per-pass budget from max_rewind_commits.
BFS returns materialized candidates and canonicalization reuse
crates/cgka-engine/src/openmls_projection.rs
Returns a parallel materialized vector from BFS and conditionally reuses it during canonicalization when no pending application messages exist.
Replay budget and BFS reuse tests
crates/cgka-engine/src/openmls_projection.rs, crates/cgka-engine/tests/distributed_convergence.rs
Adds unit coverage for replay-budget behavior and an integration test for BFS-materialized candidate reuse.

Estimated code review effort: 4 (Complex) | ~60 minutes

Daemon Lock Scoping for Execute and StreamWatch

Layer / File(s) Summary
reconcile_and_clone_runtime helper
crates/cli/src/daemon/runtime_host.rs
Adds a helper that reconciles app runtime state under the workers lock and returns a cloned runtime handle.
Runtime-host handlers use SharedDaemonWorkers
crates/cli/src/daemon/runtime_host.rs
Updates account setup, runtime command, hosted-command, and refresh paths to use &SharedDaemonWorkers and off-lock dispatch with lock-scoped reconciliation.
Connection dispatch and Execute/StreamWatch handlers
crates/cli/src/daemon/mod.rs, crates/cli/src/daemon/stream_workers.rs
Forwards shared workers into the handlers and changes Execute/StreamWatch paths to hold the lock only around reconciliation or compose mutation.
Lock-contention regression test
crates/cli/src/daemon/tests.rs
Adds a test that keeps the workers mutex held while Execute runs and asserts the call still completes.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related issues

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the two main changes: OpenMLS convergence replay reuse and daemon lock scoping.
Linked Issues check ✅ Passed The changes implement #635 replay reuse/budgeting and #633 lock scoping, matching the linked issues' objectives.
Out of Scope Changes check ✅ Passed No clear out-of-scope code changes are introduced beyond the convergence and daemon concurrency fixes in scope.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/wizardly-hellman-097779

Comment @coderabbitai help to get the list of available commands.

@stage-review

stage-review Bot commented Jul 1, 2026

Copy link
Copy Markdown

Ready to review this PR? Stage has broken it down into 3 individual chapters for you:

Title
1 Introduce ReplayBudget to bound convergence BFS
2 Enable BFS materialized candidate reuse
3 Refactor daemon workers lock scoping
Open in Stage

Chapters generated by Stage for commit ea11b5e on Jul 2, 2026 1:19am UTC.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/cli/src/daemon/mod.rs`:
- Around line 407-420: The `Command::Stream` branch in `mod.rs` is still
acquiring the `workers` mutex for all stream commands, which can block
non-compose hosted stream paths unnecessarily. Update the `if
matches!(cli.command, ...)` gate to match only the compose subcommands before
locking `workers`, and keep the lock acquisition inside that narrower
compose-only branch around `handle_stream_compose_request`.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8e8119f0-9b2b-4eea-9e09-e445eab1d439

📥 Commits

Reviewing files that changed from the base of the PR and between 8451fa9 and df9a1d0.

📒 Files selected for processing (6)
  • crates/cgka-engine/src/openmls_projection.rs
  • crates/cgka-engine/tests/distributed_convergence.rs
  • crates/cli/src/daemon/mod.rs
  • crates/cli/src/daemon/runtime_host.rs
  • crates/cli/src/daemon/stream_workers.rs
  • crates/cli/src/daemon/tests.rs

Comment thread crates/cli/src/daemon/mod.rs Outdated
#770)

The Execute arm took the workers lock for every `Command::Stream`, so non-compose
hosted stream commands (start/finish/watch/send) briefly contended on an unrelated
busy workers mutex before `handle_stream_compose_request` returned None and they fell
through to the off-lock hosted path. Narrow the gate to the compose subcommands
(ComposeOpen/Append/Finish/Cancel) so only they acquire the lock; behavior is
otherwise unchanged (the compose request already returned None for non-compose stream
commands).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@erskingardner
erskingardner merged commit 26e135c into master Jul 2, 2026
20 checks passed
@erskingardner
erskingardner deleted the claude/wizardly-hellman-097779 branch July 2, 2026 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant