Conversation
WebSocket and HTTP servers can now bind to Unix sockets via unix:// prefix or --socket flag, enabling multiple GenVM instances on the same host without port conflicts.
|
|
📝 WalkthroughWalkthroughAdds Unix domain socket support and a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as Hyper/Warp
participant Transport as Transport Layer
participant Handler as MessageHandler
rect rgba(200,150,200,0.5)
Note over Client,Server: TCP flow
Client->>Server: TCP connect
Server->>Transport: accept TCP stream
Server->>Client: WebSocket upgrade
Transport->>Handler: handle_stream (TCP)
Handler->>Handler: read_hello / message loop
Handler->>Handler: cleanup()
Transport->>Transport: TCP connection close
end
rect rgba(150,200,200,0.5)
Note over Client,Server: Unix socket flow
Transport->>Transport: pre-bind stale socket cleanup
Client->>Server: connect to Unix socket
Server->>Transport: accept Unix stream (UnixAcceptor)
Server->>Client: WebSocket upgrade
Transport->>Handler: handle_stream (Unix)
Handler->>Handler: read_hello / message loop
Handler->>Handler: cleanup()
Transport->>Transport: remove socket file on shutdown
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/implementation/src/common/mod.rs (1)
401-407:⚠️ Potential issue | 🟡 MinorMisleading log message on accept error.
When
listener.accept()returns anErr, the log says "accepted None" which is inaccurate. The actual case is an accept error, not a closed listener.Suggested improvement
accepted = listener.accept() => { if let Ok((stream, _)) = accepted { tokio::spawn(loop_one(handler_provider.clone(), stream)); } else { - log_info!("accepted None"); + log_warn!(error:? = accepted.err(); "accept failed"); return Ok(()) } }
🤖 Fix all issues with AI agents
In `@modules/implementation/src/common/mod.rs`:
- Around line 453-459: The accept branch in the async Unix listener incorrectly
logs "accepted None" on Err; update the listener.accept handling (the match that
currently spawns loop_one_unix(handler_provider.clone(), stream)) to capture the
Err as e and log the actual error (e) instead of "accepted None"—use the
appropriate logging macro (e.g., log_error! or log_info! with error details) and
keep the early return behavior after logging.
🧹 Nitpick comments (1)
modules/implementation/src/manager/mod.rs (1)
332-335: Potential TOCTOU when removing stale socket.The check-then-remove pattern has a small race window where another process could create the socket between
exists()andremove_file(). This is generally acceptable for local development/testing scenarios described in the PR, but be aware that in production with rapid restarts, this could inadvertently remove an active socket from a still-running process.Consider documenting this behavior or using advisory file locking if stricter guarantees are needed in the future.
| accepted = listener.accept() => { | ||
| if let Ok((stream, _)) = accepted { | ||
| tokio::spawn(loop_one_unix(handler_provider.clone(), stream)); | ||
| } else { | ||
| log_info!("accepted None"); | ||
| return Ok(()) | ||
| } |
There was a problem hiding this comment.
Same misleading log message on accept error.
This has the same issue as the TCP version - the "accepted None" log is inaccurate when the actual case is an accept error.
Suggested improvement
accepted = listener.accept() => {
if let Ok((stream, _)) = accepted {
tokio::spawn(loop_one_unix(handler_provider.clone(), stream));
} else {
- log_info!("accepted None");
+ log_warn!(error:? = accepted.err(); "accept failed");
return Ok(())
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| accepted = listener.accept() => { | |
| if let Ok((stream, _)) = accepted { | |
| tokio::spawn(loop_one_unix(handler_provider.clone(), stream)); | |
| } else { | |
| log_info!("accepted None"); | |
| return Ok(()) | |
| } | |
| accepted = listener.accept() => { | |
| if let Ok((stream, _)) = accepted { | |
| tokio::spawn(loop_one_unix(handler_provider.clone(), stream)); | |
| } else { | |
| log_warn!(error:? = accepted.err(); "accept failed"); | |
| return Ok(()) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@modules/implementation/src/common/mod.rs` around lines 453 - 459, The accept
branch in the async Unix listener incorrectly logs "accepted None" on Err;
update the listener.accept handling (the match that currently spawns
loop_one_unix(handler_provider.clone(), stream)) to capture the Err as e and log
the actual error (e) instead of "accepted None"—use the appropriate logging
macro (e.g., log_error! or log_info! with error details) and keep the early
return behavior after logging.
kp2pml30
left a comment
kp2pml30
left a comment
There was a problem hiding this comment.
lgtm, thanks, I wanted to add it but forgot :)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/implementation/src/common/mod.rs (1)
401-410:⚠️ Potential issue | 🟡 MinorMisleading log message on TCP accept error.
When
listener.accept()returns anErr, logging "accepted None" is inaccurate. The actual error should be logged for debugging.Suggested fix
accepted = listener.accept() => { if let Ok((stream, _)) = accepted { tokio::spawn(loop_one(handler_provider.clone(), stream)); } else { - log_info!("accepted None"); + log_warn!(error:? = accepted.err(); "accept failed"); return Ok(()) } }
🧹 Nitpick comments (1)
modules/implementation/src/common/mod.rs (1)
451-472: Consider simplifying by removing unnecessary variable binding.The
resultvariable binding and explicit return add no value here - the async block can be awaited directly.Suggested simplification
- let result = async { - loop { - tokio::select! { - _ = cancel.chan.closed() => { - log_info!("loop cancelled"); - return Ok(()) - } - accepted = listener.accept() => { - if let Ok((stream, _)) = accepted { - tokio::spawn(loop_one_unix(handler_provider.clone(), stream)); - } else { - log_info!("accepted None"); - return Ok(()) - } - } + loop { + tokio::select! { + _ = cancel.chan.closed() => { + log_info!("loop cancelled"); + return Ok(()) + } + accepted = listener.accept() => { + if let Ok((stream, _)) = accepted { + tokio::spawn(loop_one_unix(handler_provider.clone(), stream)); + } else { + log_warn!(error:? = accepted.err(); "accept failed"); + return Ok(()) + } } } } - .await; - - result }
kp2pml30
deleted the
feat/nod-700-request-unix-socket-binding-support-in-genvm-manager
branch
3 participants
Description
Adds Unix domain socket support to GenVM manager and modules as an alternative to TCP binding.
This enables running multiple GenVM instances on the same host without port conflicts, which is
essential for multi-validator local running for zero-downtime upgrade and testing and worktree development workflows.
Changes:
--socketCLI flag (mutually exclusive with--host/--port)unix://prefix inbind_addressconfigurationUsage:
Types of Changes
Checklist
Summary by CodeRabbit
New Features
Chores