fix: update tests/examples for upstream API changes and fix sync loop busy-loop#3057
Conversation
Align error types, struct fields, and constructor calls in 27 test/example files to match current Core API: - Box<dyn Error> -> Box<dyn Error + Send + Sync> to match Core::new()/shutdown() - entry::ActiveModel device_id -> volume_id (field rename) - location::ActiveModel add volume_id field (new nullable field) - AppConfig add proxy_pairing and spacebot fields (config v5/v6) - Re-export ProxyPairingConfig from config module - VolumeFingerprint::new() -> from_primary/external/network_volume() - DeviceInfo add device_type field, NetworkFingerprint public_key -> public_key_hash - SecretKey::generate(thread_rng) -> from_bytes (rand_core version mismatch) - SdPath::Physical.path String -> PathBuf, thumbnail_url &Uuid -> &str - init_sync_service unwrap networking Option for Arc<dyn NetworkTransport>
WalkthroughThis PR broadens boxed error trait bounds to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
core/tests/relay_pairing_test.rs (1)
156-161: Use a fixed seed to keep this test reproducible.The current seed comes from
thread_rng(), so test inputs vary per run. A fixed non-zero 32-byte seed makes failures reproducible and keeps behavior stable.Proposed change
- use rand::RngCore; - - // Use from_bytes to avoid CryptoRng trait bound mismatch between rand 0.8 and iroh's rand_core - let mut seed = [0u8; 32]; - rand::thread_rng().fill_bytes(&mut seed); - let secret_key = SecretKey::from_bytes(&seed); + // Use a fixed seed to keep test behavior deterministic. + let seed = [7u8; 32]; + let secret_key = SecretKey::from_bytes(&seed);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/tests/relay_pairing_test.rs` around lines 156 - 161, The test uses rand::thread_rng().fill_bytes(&mut seed) which makes the seed non-deterministic; replace this with a fixed non-zero 32-byte seed to make the test reproducible (e.g., set seed = [1u8; 32] or a specific byte array) before calling SecretKey::from_bytes(&seed), updating the code around the seed variable and the SecretKey::from_bytes call to use the deterministic seed instead of thread_rng.core/tests/proxy_pairing_protocol_test.rs (1)
182-185: Derivepublic_key_hashfrom the test public key instead of hard-coding it.Using a literal hash that is unrelated to
vouchee_public_keymakes the payload fixture internally inconsistent and weakens this test’s realism. Prefer computingpublic_key_hashfrom the same key bytes used invouchee_public_key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/tests/proxy_pairing_protocol_test.rs` around lines 182 - 185, The test currently hard-codes network_fingerprint.public_key_hash; instead derive it from the same vouchee_public_key used in the fixture so the payload is consistent. Replace the literal "abcdef..." with a computed value by hashing/parsing the vouchee_public_key bytes using the identity module’s helper (use the same function used in production in sd_core::service::network::utils::identity to compute a public key hash) and assign that result to NetworkFingerprint.public_key_hash so network_fingerprint and vouchee_public_key remain consistent.core/tests/volume_fingerprint_stability_test.rs (1)
61-63: Usetracingmacros instead ofprintln!for logging, but note that test files in this codebase predominantly useprintln!for test output.While the coding guidelines recommend using
tracingmacros for all.rsfiles (includingcore/**/*.rs), an analysis ofcore/tests/shows that 38 test files useprintln!compared to 23 using tracing macros. The current approach involume_fingerprint_stability_test.rsaligns with the predominant test logging convention. If refactored to use tracing, you would need to add imports and ensure a tracing subscriber is initialized for the test context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/tests/volume_fingerprint_stability_test.rs` around lines 61 - 63, The test currently uses println! for output (calls to fp_vol1.short_id() and fp_vol2.short_id()); change those println! calls to a tracing macro (e.g., tracing::info!) and ensure tracing is initialized in the test (add the tracing import and initialize a subscriber once for the test context, e.g., via tracing_subscriber::fmt::init() or a test-wide once setup) so the test logs appear when run under the tracing system.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core/tests/proxy_pairing_protocol_test.rs`:
- Around line 182-185: The test currently hard-codes
network_fingerprint.public_key_hash; instead derive it from the same
vouchee_public_key used in the fixture so the payload is consistent. Replace the
literal "abcdef..." with a computed value by hashing/parsing the
vouchee_public_key bytes using the identity module’s helper (use the same
function used in production in sd_core::service::network::utils::identity to
compute a public key hash) and assign that result to
NetworkFingerprint.public_key_hash so network_fingerprint and vouchee_public_key
remain consistent.
In `@core/tests/relay_pairing_test.rs`:
- Around line 156-161: The test uses rand::thread_rng().fill_bytes(&mut seed)
which makes the seed non-deterministic; replace this with a fixed non-zero
32-byte seed to make the test reproducible (e.g., set seed = [1u8; 32] or a
specific byte array) before calling SecretKey::from_bytes(&seed), updating the
code around the seed variable and the SecretKey::from_bytes call to use the
deterministic seed instead of thread_rng.
In `@core/tests/volume_fingerprint_stability_test.rs`:
- Around line 61-63: The test currently uses println! for output (calls to
fp_vol1.short_id() and fp_vol2.short_id()); change those println! calls to a
tracing macro (e.g., tracing::info!) and ensure tracing is initialized in the
test (add the tracing import and initialize a subscriber once for the test
context, e.g., via tracing_subscriber::fmt::init() or a test-wide once setup) so
the test logs appear when run under the tracing system.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 69d03af5-7ce3-4a1b-ae8c-e18372bd4d89
📒 Files selected for processing (27)
core/examples/create_memory.rscore/examples/file_type_demo.rscore/examples/indexing_demo.rscore/examples/job_logging_test.rscore/examples/library_demo.rscore/examples/pause_resume_demo.rscore/examples/shutdown_demo.rscore/examples/simple_pause_resume.rscore/examples/test_migration.rscore/examples/volume_demo.rscore/src/config/mod.rscore/tests/ephemeral_watcher_test.rscore/tests/event_system_test.rscore/tests/file_structure_test.rscore/tests/file_sync_simple_test.rscore/tests/file_sync_test.rscore/tests/fs_watcher_test.rscore/tests/job_resumption_integration_test.rscore/tests/job_shutdown_test.rscore/tests/normalized_cache_fixtures_test.rscore/tests/phase_snapshot_test.rscore/tests/proxy_pairing_protocol_test.rscore/tests/relay_pairing_test.rscore/tests/resource_events_test.rscore/tests/transitive_sync_backfill_test.rscore/tests/volume_fingerprint_stability_test.rscrates/sd-client/examples/test_connection.rs
The sync loop's Ready state used 'continue' when is_realtime_active() returned true, which skipped the sleep at the bottom of the loop and caused a CPU-saturating busy-loop logging 'Skipping catch-up' hundreds of thousands of times per second. Replace with if/else so the realtime-active branch falls through to the configurable sleep.
Summary
Core::new()/Core::shutdown()signatures (Box<dyn Error + Send + Sync>)entry::ActiveModelfielddevice_idtovolume_idand add missingvolume_idtolocation::ActiveModelinlibrary_demo.rsproxy_pairingandspacebotfields toAppConfiginitializers in sync testsVolumeFingerprinttests to use new constructors (from_primary_volume,from_external_volume,from_network_volume)SecretKey::generate()in relay pairing test to usefrom_bytespattern (rand_core 0.9 compatibility)SdPath::Physical.pathtype (PathBuf, not String) andthumbnail_urlparameter type in sd-client exampleProxyPairingConfigre-export fromconfig/mod.rsDeviceInfoandNetworkFingerprintstruct fields in proxy pairing protocol testinit_sync_servicecalls to unwrapOption<Arc<NetworkingService>>continuein theReadystate skipped the sleep whenis_realtime_active()was true, causing CPU-saturating spin. Replaced withif/elseto fall through to sleep.Root Cause
The core library (
cargo build --lib -p sd-core) compiles fine, but API changes weren't propagated to examples and tests. Key changes:Core::new()/shutdown()error type gainedSend + Sync,entry::Modelrenameddevice_idtovolume_id,VolumeFingerprint::new()removed in favor of purpose-specific constructors,AppConfiggainedproxy_pairingandspacebotfields, andirohupgradedrand_corebreakingSecretKey::generate()withThreadRng.Additionally, a production bug in
run_sync_loop()caused a busy-loop when real-time sync was active:continueskipped the sleep at the bottom of the loop.Changes
core/examples/,core/tests/,core/src/config/,core/src/service/sync/, andcrates/sd-client/examples/