Fix daemon synthesis responses and release idle Core memory#134
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses stability and resource management issues within the daemon server. By adjusting the IPC frame limits, it ensures that larger synthesis outputs do not prematurely terminate client connections. Additionally, it optimizes memory footprint by moving the VOICEVOX Core lifecycle to a per-request model and implementing platform-specific memory pressure relief, which is particularly beneficial for macOS environments. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. The daemon's memory was growing wide, With models lingering deep inside. We freed the space with a clever hook, And gave the frame limits a second look. Footnotes
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 18 minutes and 42 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR implements deferred VoicevoxCore initialization in the daemon synthesis executor, adds a platform-specific memory release mechanism, updates IPC frame size configuration to respect both request and response limits, and refactors model unloading to derive voice model IDs directly. Changes
Sequence DiagramsequenceDiagram
actor Client
participant DaemonState
participant Executor
participant VoicevoxCore
participant Memory
rect rgba(100, 150, 200, 0.5)
Note over DaemonState: Initialization (catalog-only)
DaemonState->>VoicevoxCore: new()
VoicevoxCore-->>DaemonState: core instance
DaemonState->>DaemonState: discover models
DaemonState->>VoicevoxCore: drop
DaemonState->>Memory: release_unused_allocator_memory()
end
rect rgba(150, 200, 100, 0.5)
Note over Client: Synthesis Request
Client->>Executor: synthesize(model_id, text)
Executor->>Executor: ensure_core() called
Executor->>VoicevoxCore: new() [lazy init]
VoicevoxCore-->>Executor: core instance stored
Executor->>VoicevoxCore: load model
Executor->>VoicevoxCore: synthesize
VoicevoxCore-->>Executor: audio output
Executor->>Executor: release_core()
Executor->>VoicevoxCore: drop
Executor->>Memory: release_unused_allocator_memory()
Executor-->>Client: Result<audio>
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Pull request overview
This PR adjusts the daemon IPC framing to allow large synthesized WAV responses without disconnecting, and changes daemon/Core lifecycle to release VOICEVOX Core/model memory between requests (with an additional macOS-specific allocator relief hook).
Changes:
- Add an infrastructure
memorymodule with a macOS allocator pressure-relief hook (no-op elsewhere). - Make VOICEVOX Core a per-request resource in the daemon synthesis executor; release Core and attempt allocator relief after synthesis/unload.
- Increase the daemon server’s length-delimited frame limit to accommodate the larger response-frame budget from the IPC contract.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/infrastructure/mod.rs |
Exposes the new memory module. |
src/infrastructure/memory.rs |
Adds macOS allocator relief hook + non-macOS no-op. |
src/infrastructure/daemon/state/policy.rs |
Adjusts locking to allow mutable synthesis executor access. |
src/infrastructure/daemon/state/executor.rs |
Switches to per-request Core init/teardown and calls allocator relief on teardown. |
src/infrastructure/daemon/state.rs |
Drops the catalog Core after building the model catalog and triggers allocator relief. |
src/infrastructure/daemon/server.rs |
Raises codec max frame length to support larger response frames. |
src/infrastructure/core.rs |
Calls allocator relief after unloading a model. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/infrastructure/memory.rs (1)
11-17: Add a SAFETY comment to document the FFI contract.The unsafe FFI block lacks explicit documentation of why the symbol and null-zone call are valid. While the repository does not enforce unsafe-block documentation via linting, adding a SAFETY comment follows Rust best practices and clarifies intent.
Proposed safety-comment addition
- unsafe { malloc_zone_pressure_relief(std::ptr::null_mut(), 0) as usize } + // SAFETY: `malloc_zone_pressure_relief` is provided by macOS libmalloc. + // Passing a null zone asks libmalloc to inspect all zones, and `goal = 0` + // requests best-effort relief without transferring ownership. + unsafe { malloc_zone_pressure_relief(std::ptr::null_mut(), 0) as usize }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/infrastructure/memory.rs` around lines 11 - 17, Add a SAFETY comment documenting the FFI contract for the unsafe extern "C" declaration of malloc_zone_pressure_relief and its subsequent call with std::ptr::null_mut() and 0: state that the symbol is provided by libmalloc (macOS), that passing a null zone is valid and requests global pressure relief, that goal==0 requests maximum possible relief, that the function uses the C calling convention and is safe to call from this thread context, and explain why casting the returned size_t to usize is valid; place this comment immediately above the unsafe extern "C" block and the unsafe call to malloc_zone_pressure_relief to clearly justify the unsafety and required invariants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/infrastructure/daemon/server.rs`:
- Around line 120-123: The current code creates a single LengthDelimitedCodec
and Framed for both read/write, which lets the decoder use the larger
MAX_DAEMON_RESPONSE_FRAME_BYTES limit; change to separate read and write codecs
so decoding is capped by MAX_DAEMON_REQUEST_FRAME_BYTES and encoding uses
MAX_DAEMON_RESPONSE_FRAME_BYTES: build two codecs with
LengthDelimitedCodec::builder().max_frame_length(...) (one using
MAX_DAEMON_REQUEST_FRAME_BYTES, the other using
MAX_DAEMON_RESPONSE_FRAME_BYTES), split the stream into read/write halves (or
use FramedRead/FramedWrite) to produce framed_read and framed_write, then read
from framed_read.next() and send responses via framed_write.send(...).
In `@src/infrastructure/daemon/state/executor.rs`:
- Around line 60-62: release_core currently sets self.core = None before calling
crate::infrastructure::memory::release_unused_allocator_memory(), which leaves
Core alive on the stack during unwinding; change release_core so it takes the
Core out (e.g., let core = self.core.take()) into a local variable and
create/associate an allocator-relief RAII guard (similar to ModelUnloadGuard) so
that when the local core goes out of scope it is dropped before you call
release_unused_allocator_memory(); ensure ensure_core and any places covered by
the same pattern (the logic repeated around the 83-114 range) are updated to use
the same take-into-local + drop-then-release sequence so native allocations are
freed even during panic unwinding.
---
Nitpick comments:
In `@src/infrastructure/memory.rs`:
- Around line 11-17: Add a SAFETY comment documenting the FFI contract for the
unsafe extern "C" declaration of malloc_zone_pressure_relief and its subsequent
call with std::ptr::null_mut() and 0: state that the symbol is provided by
libmalloc (macOS), that passing a null zone is valid and requests global
pressure relief, that goal==0 requests maximum possible relief, that the
function uses the C calling convention and is safe to call from this thread
context, and explain why casting the returned size_t to usize is valid; place
this comment immediately above the unsafe extern "C" block and the unsafe call
to malloc_zone_pressure_relief to clearly justify the unsafety and required
invariants.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 49d0a0f5-2c23-4a2d-8490-95e6e4f8349e
📒 Files selected for processing (7)
src/infrastructure/core.rssrc/infrastructure/daemon/server.rssrc/infrastructure/daemon/state.rssrc/infrastructure/daemon/state/executor.rssrc/infrastructure/daemon/state/policy.rssrc/infrastructure/memory.rssrc/infrastructure/mod.rs
Why
Longer synthesis results could exceed the daemon server's write-side frame limit and close the connection, which surfaced to clients as "No response from daemon" even though the daemon process stayed alive. On macOS Tohae, unloaded VOICEVOX models also left the daemon with higher resident memory than expected between requests.
What
Use the daemon's declared response-frame budget on the server side as well as the client side, preserving the smaller request limit while allowing synthesized WAV responses to use the larger response limit already defined by the IPC contract. Keep the no-model-cache policy by making VOICEVOX Core a per-request resource instead of daemon-idle state; this trades some startup work per synthesis for predictable resident memory, which matches the existing design preference for bounded memory over latency. Add a small infrastructure-level allocator relief hook so macOS can return freed native allocations to the OS after model/Core teardown, while non-macOS platforms keep a no-op path.
Summary by CodeRabbit
Bug Fixes
Chores