fix(p2p): serialize sync application - #73
Merged
Merged
Conversation
Signed-off-by: trvon <git@trevon.dev>
There was a problem hiding this comment.
🟡 Changes recommended
The updated MemorySyncService shutdown/drain locking introduces a credible cross-thread ABBA deadlock scenario between lifecycleMutex_ and the global callbackExecutionMutex_ that should be resolved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds process-wide serialization for MemorySyncService post-sync adapter callbacks and serializes the daemon’s memory-sync apply/backfill pipelines to prevent concurrent callback/apply interleavings that can trigger cross-service lock cycles during direct-P2P replication.
Changes:
- Serialize MemorySyncService after-sync callback execution across service instances, with callback coalescing for recursive callback-triggered work and stop/restart generation invalidation.
- Serialize daemon memory-sync apply and backfill execution with dedicated mutexes and test-only counters/lock-introspection.
- Add unit and integration tests covering concurrent apply/callback serialization, stop/drain behavior, and callback-driven cross-service interactions.
File summaries
| File | Description |
|---|---|
| tests/unit/memory_sync/memory_sync_service_catch2_test.cpp | Adds concurrency/stop/destruction/recursive-callback unit tests for serialized post-sync callbacks. |
| tests/integration/daemon/meson.build | Registers a new non-parallel daemon integration test for apply serialization. |
| tests/integration/daemon/memory_sync_service_integration_test.cpp | Adds integration coverage for serialized daemon apply/backfill callbacks and correct replication outcomes. |
| src/daemon/components/ServiceManager.cpp | Serializes daemon apply/backfill entrypoints and adds attempt counters for test assertions. |
| include/yams/memory_sync/memory_sync_service.h | Implements process-wide callback serialization, callback snapshot context stacking, and stop/restart callback drain logic. |
| include/yams/daemon/components/ServiceManager.h | Adds mutexes/counters and testing helpers for daemon apply/backfill serialization observability. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
132
to
+146
| @@ -117,6 +141,9 @@ class MemorySyncService { | |||
| shutdownFailures_.fetch_add(1, std::memory_order_relaxed); | |||
| } | |||
| } | |||
| // Direct-session callbacks do not belong to worker_. Drain the active callback before | |||
| // releasing lifecycleMutex_. Generation checks discard callbacks queued before stop(). | |||
| std::lock_guard<std::recursive_mutex> callbackDrain(callbackExecutionMutex_); | |||
Comment on lines
+567
to
573
| while (!callbackExecutionMutex_.try_lock()) { | ||
| if (stop_.load(std::memory_order_acquire) || | ||
| callbackGeneration != callbackGeneration_.load(std::memory_order_acquire)) { | ||
| return; | ||
| } | ||
| std::this_thread::yield(); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked draft child of #72. This change closes the disclosed direct-P2P replicated-apply serialization blocker without merging or deploying anything.
Before the fix:
maximumActive == 2)apply.after_contentwhile the first callback remained blockedKnown stack blockers
This draft child is not independently mergeable or deployable. The umbrella still has separately tracked operator-status semantics, fuzzing, and final live/release gates.
AI disclosure
Extensive AI assistance was used for implementation, test generation, and adversarial review. All changes were locally inspected and validated; remaining stack blockers are disclosed above.
Stack
fix/p2p-bootstrap-trust/ fix(p2p): harden bootstrap trust #72 at435ff22518f206bb77617e312d1962cb573eec38fix/p2p-apply-serializationated7b09a23cf1bfb5cb6be0bcfc769d826f9e82b4Do not merge or deploy this child independently.