fix: prevent FFI event flooding and memory leak in progress callbacks #173
Conversation
This commit addresses two critical issues in the FFI layer: 1. Event Throttling: Limit event draining to 500 events per call to prevent UI/main thread flooding. Remaining events stay queued for the next drain. 2. Memory Leak Fix: Properly free heap-allocated stage_message strings in FFIDetailedSyncProgress after each progress callback to avoid per-callback memory leaks. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
WalkthroughDrain loop in the FFIDashSpvClient now limits processed events to 500 per call, leaving remaining events queued. Sync-to-tip progress uses a stack-allocated Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EventQueue
participant MainThread
rect rgb(240,248,255)
Note right of Client: drain_events_internal
Client->>EventQueue: pull next event (loop)
alt processed < 500
EventQueue-->>Client: event
Client->>MainThread: dispatch event
Client->>EventQueue: continue loop
else reached 500
EventQueue-->>Client: event (processed 500)
Client->>MainThread: dispatch event
Client-->>EventQueue: stop (leave remaining queued)
end
end
sequenceDiagram
participant Syncer
participant FFI_Callback
participant Heap
rect rgb(255,250,240)
Note right of Syncer: sync_to_tip_with_progress
Syncer->>Syncer: create stack FFIDetailedSyncProgress (mut)
Syncer->>FFI_Callback: invoke callback with &ffi_progress
alt callback may set stage_message (heap string)
FFI_Callback-->>Syncer: returns
Syncer->>Heap: move & destroy stage_message heap allocation
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dash-spv-ffi/src/client.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
dash-spv-ffi/src/**/*.rs
📄 CodeRabbit inference engine (dash-spv-ffi/CLAUDE.md)
dash-spv-ffi/src/**/*.rs: FFI functions must be declared with #[no_mangle] extern "C"
Expose complex Rust types to C as opaque pointers (e.g., FFIDashSpvClient*)
Provide corresponding _destroy() functions for all FFI-owned types to enforce explicit memory management
Return Rust strings to C as *const c_char; allocate so the caller can free with dash_string_free
Accept input strings from C as *const c_char (borrowed; do not free inputs)
Functions returning pointers transfer ownership to the caller; functions taking pointers only borrow
Add cbindgen annotations for complex types so headers are generated correctly
Files:
dash-spv-ffi/src/client.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use proper error types with thiserror and propagate errors appropriately
Use the tokio runtime for async operations in Rust
Use conditional compilation feature flags for optional features (#[cfg(feature = ...)])
Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Run clippy with -D warnings and fix all lints
Adhere to MSRV Rust 1.89 (avoid features requiring newer compiler)
**/*.rs: Format Rust code with rustfmt (per rustfmt.toml); run cargo fmt --all before commits
Lint with clippy; treat warnings as errors in CI
Follow Rust naming: snake_case for functions/variables, UpperCamelCase for types/traits, SCREAMING_SNAKE_CASE for consts
Prefer async using tokio where applicable
Files:
dash-spv-ffi/src/client.rs
{dash-network-ffi,dash-spv-ffi,key-wallet-ffi,swift-dash-core-sdk}/**/*.{rs,c,h,swift}
📄 CodeRabbit inference engine (CLAUDE.md)
Be careful with memory management at FFI boundaries (C/Swift interop)
Files:
dash-spv-ffi/src/client.rs
**/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/src/**/*.rs: Each crate keeps sources in src/
Avoid unwrap()/expect() in library code; use proper error types (e.g., thiserror)
Place unit tests alongside code with #[cfg(test)]
Files:
dash-spv-ffi/src/client.rs
*-ffi/**
📄 CodeRabbit inference engine (AGENTS.md)
FFI bindings live in *-ffi crates
Files:
dash-spv-ffi/src/client.rs
🧠 Learnings (14)
📓 Common learnings
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:49:53.811Z
Learning: In Rust FFI code, using thread-local storage for error reporting is unsafe in multi-threaded contexts, as callbacks may execute on different threads than the caller, leading to lost or invisible errors. FFI error handling should use global, thread-safe storage or return errors directly via return values or out parameters.
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Expose complex Rust types to C as opaque pointers (e.g., FFIDashSpvClient*)
Applied to files:
dash-spv-ffi/src/client.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Provide corresponding _destroy() functions for all FFI-owned types to enforce explicit memory management
Applied to files:
dash-spv-ffi/src/client.rs
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/*.rs : All code must be clippy-clean: run `cargo clippy --all-targets --all-features -- -D warnings`
Applied to files:
dash-spv-ffi/src/client.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Accept input strings from C as *const c_char (borrowed; do not free inputs)
Applied to files:
dash-spv-ffi/src/client.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/callbacks.rs : Use C function pointers for async progress/completion callbacks; callbacks may be invoked from any thread
Applied to files:
dash-spv-ffi/src/client.rs
📚 Learning: 2025-08-29T04:00:14.154Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T04:00:14.154Z
Learning: Applies to {dash-network-ffi,dash-spv-ffi,key-wallet-ffi,swift-dash-core-sdk}/**/*.{rs,c,h,swift} : Be careful with memory management at FFI boundaries (C/Swift interop)
Applied to files:
dash-spv-ffi/src/client.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/include/dash_spv_ffi.h : The public C header is generated; ensure it reflects FFI changes after builds
Applied to files:
dash-spv-ffi/src/client.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : FFI functions must be declared with #[no_mangle] extern "C"
Applied to files:
dash-spv-ffi/src/client.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Return Rust strings to C as *const c_char; allocate so the caller can free with dash_string_free
Applied to files:
dash-spv-ffi/src/client.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Functions returning pointers transfer ownership to the caller; functions taking pointers only borrow
Applied to files:
dash-spv-ffi/src/client.rs
📚 Learning: 2025-06-26T15:49:53.811Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:49:53.811Z
Learning: In Rust FFI code, using thread-local storage for error reporting is unsafe in multi-threaded contexts, as callbacks may execute on different threads than the caller, leading to lost or invisible errors. FFI error handling should use global, thread-safe storage or return errors directly via return values or out parameters.
Applied to files:
dash-spv-ffi/src/client.rs
📚 Learning: 2025-08-16T04:15:29.335Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:29.335Z
Learning: Applies to swift-dash-core-sdk/Sources/**/FFIBridge.swift : Manage FFI pointers manually using RAII-style wrappers and defer blocks to ensure cleanup
Applied to files:
dash-spv-ffi/src/client.rs
📚 Learning: 2025-08-16T04:15:29.335Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:29.335Z
Learning: Applies to swift-dash-core-sdk/Sources/**/AsyncBridge.swift : Treat callbacks from C as potentially arriving on any thread and ensure thread-safe progress tracking within AsyncBridge
Applied to files:
dash-spv-ffi/src/client.rs
🧬 Code graph analysis (1)
dash-spv-ffi/src/client.rs (1)
dash-spv-ffi/src/types.rs (1)
dash_spv_ffi_string_destroy(334-338)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: SPV Components Tests
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_outpoint_string)
🔇 Additional comments (1)
dash-spv-ffi/src/client.rs (1)
242-249: LGTM! Event throttling implementation is correct.The event throttling logic properly limits processing to 500 events per
drain_events_internalcall, preventing UI thread flooding. The counter is correctly incremented only after successful event processing, and remaining events stay queued for subsequent drain calls.Also applies to: 364-364
This commit enhances memory management in the FFI layer by ensuring that the stage_message is properly moved out of the FFIDetailedSyncProgress struct before destruction. This change prevents potential double-free issues and ensures that heap-allocated strings are correctly freed, addressing memory leak concerns during progress callbacks.
This commit updates the comment regarding the handling of ffi_progress in the FFI layer. It clarifies that ffi_progress will be dropped normally, as there is no Drop implementation, improving the understanding of memory management in the context of progress callbacks.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This commit addresses two critical issues in the FFI layer:
Event Throttling: Limit event draining to 500 events per call to prevent UI/main thread flooding. Remaining events stay queued for the next drain.
Memory Leak Fix: Properly free heap-allocated stage_message strings in FFIDetailedSyncProgress after each progress callback to avoid per-callback memory leaks.
🤖 Generated with Claude Code
Summary by CodeRabbit