refactor(p2p): tighten tests, share helpers, and guard test hooks - #77
Merged
Conversation
Compile the direct-P2P test-only machinery out of production builds (YAMS_DAEMON_TEST_HOOKS_ENABLED now wraps the reconnect-loop hook storage, invoker, and call site, matching the already-guarded setter). Replace the single shared writer key in p2p_manager tests with one distinct key pair per writer identity so origin-binding regressions (a record claiming origin X but signed by Y's key) fail verification. Extract the copy-pasted bytes/text/TempDir helpers into tests/common/p2p_test_helpers.h and reuse them across the transport/delta/manager suites. Replace a timing-sensitive fixed sleep in the transport session-expiry test with a bounded poll that accepts both deadline manifestations, and gate the memory-sync-service concurrent-lifecycle stress case behind YAMS_RUN_STRESS_TESTS=1 per the existing FTS5 pattern. Deduplicate the byte-identical commitment-JSON serialization inside MemorySyncLoop (coldBootstrapRoot / persistReplicationCheckpoint) into one static helper. Signed-off-by: trvon <git@trevon.dev>
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of concrete test-code issues to fix (helper header not self-contained due to missing <cstddef>, and stress gating semantics not matching the documented =1/true convention).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR tightens and refactors the direct-P2P and memory-sync test surface while ensuring daemon-side test-hook machinery is compiled out of production builds, and it deduplicates a byte-identical commitments JSON serialization helper in MemorySyncLoop.
Changes:
- Compile-time guard reconnect-loop test hook storage/invocation behind
YAMS_DAEMON_TEST_HOOKS_ENABLED. - Extract shared P2P unit-test helpers (
bytes,text,TempDir) intotests/common/p2p_test_helpers.hand reuse across P2P suites; make the session-deadline test less timing-sensitive by polling rather than sleeping. - Add opt-in gating for a stress-tagged
MemorySyncServicetest viaYAMS_RUN_STRESS_TESTS, and deduplicate commitments JSON construction inMemorySyncLoop.
File summaries
| File | Description |
|---|---|
| tests/unit/memory_sync/memory_sync_service_catch2_test.cpp | Adds env-gated skipping for a stress lifecycle/status race test. |
| tests/unit/daemon/p2p_transport_catch2_test.cpp | Switches to shared helpers and replaces fixed sleep with bounded polling for session-expiry verification. |
| tests/unit/daemon/p2p_manager_catch2_test.cpp | Switches to shared helpers and strengthens writer-auth test setup with per-writer key material. |
| tests/unit/daemon/p2p_delta_catch2_test.cpp | Switches to shared helpers to remove duplicated byte/text/tempdir utilities. |
| tests/common/p2p_test_helpers.h | Introduces shared helper utilities for P2P unit tests. |
| src/daemon/p2p/p2p_manager.cpp | Guards reconnect-loop test hook storage + invocation behind YAMS_DAEMON_TEST_HOOKS_ENABLED. |
| include/yams/memory_sync/memory_sync.h | Deduplicates commitments JSON serialization via a shared helper to keep output byte-identical. |
Review details
- Files reviewed: 7/7 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
+9
to
+14
| #include <chrono> | ||
| #include <cstring> | ||
| #include <filesystem> | ||
| #include <string> | ||
| #include <string_view> | ||
| #include <vector> |
Comment on lines
+542
to
+544
| if (!yams::config::getenv_optional("YAMS_RUN_STRESS_TESTS").has_value()) { | ||
| SKIP("Stress tests disabled. Set YAMS_RUN_STRESS_TESTS=1 to enable."); | ||
| } |
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 #76. Closes the remaining code-quality / test-quality findings from the deep review of the #68 stack that were not security-blocking. The large
memory_sync.hbody split is intentionally deferred to its own follow-up (it is a ~3,400-line mechanical move best reviewed in isolation). No merge or deploy.gReconnectLoopHookstorage,invokeReconnectLoopHook, and the reconnect-loop call site are now guarded byYAMS_DAEMON_TEST_HOOKS_ENABLED, matching the already-guardedtesting_setReconnectLoopHook. Production builds no longer carry the test-hook machinery.testWriterAuthenticatorpreviously signed every node with one shared key and trusted every node with that same key, masking origin-binding regressions. Each writer id (node/client-node/server-node) now has its own key pair, and each authenticator trusts every writer with that writer's own key, so a record claiming originserver-nodebut signed withclient-node's key now fails verification. Constructor signature unchanged; zero call-site churn.bytes/text/TempDirhelpers are extracted totests/common/p2p_test_helpers.h(namespace yams::p2p_test,inline) and reused by the transport/delta/manager suites.[stress]opt-in (P2) — the memory-sync-service concurrent-lifecycle race test nowSKIPs unlessYAMS_RUN_STRESS_TESTS=1, matching the repo's existing FTS5 stress pattern, so the default suite no longer runs stress coverage.commitmentsarray serialization inMemorySyncLoop::coldBootstrapRootandpersistReplicationCheckpointis now one staticcommitmentsJson()helper. Output is byte-identical (verified by the cold-bootstrap root-hash tests, 838/60 passing).Validation
Focused normal matrix (green):
p2p_transport261 / 18 (×3 runs, deterministic)p2p_delta301 / 10 ·p2p_manager180 / 12 ·p2p_protocol495 / 14 ·peer_registry91 / 6memory_sync838 / 60 (cold-bootstrap root hashes unchanged ⇒ byte-identical JSON)memory_sync_service260 assertions / 26 passed + 1 skipped (the[stress]case now skips by default)Formatting,
git diff --check, license, Windows-header, and portability checks pass. commitlint passes; the localdcodefault range includes 119 pre-DCO experimental commits (pre-existing; the pushed range is what the pre-push hook validates).Ordinary pre-push gate passed without bypass: cross-compile smoke, ASAN 195/195, TSAN 280/280. Evidence:
build/local-ci/pre-push-ci-gate/summary-20260830T141716Z-62734.md.AI disclosure
Extensive AI assistance was used for implementation, test generation, and adversarial review. All changes were locally inspected and validated; the umbrella #68 remains blocked by the decision-grade live-performance gate.
Stack
fix/p2p-robustness/ fix(p2p): harden CLI status and operator/key-rotation flows #76 atcd956acdbee34e6c560af1a47fdfc1057ad7fa50fix/p2p-qualityata7d73d2509c51f320d1237c6c57f71190185fec9Deferred follow-up: split the ~3,400-line inline
MemorySyncLoopbodies frominclude/yams/memory_sync/memory_sync.hintosrc/memory_sync/memory_sync.cpp, and deduplicate the cross-file commitment parse sites (bounds differ per caller).Do not merge or deploy this child independently.