Repository navigation
refactor #260: expunge proto::ClientResult from user-facing API - #389
Merged
Merged
Conversation
Replace proto::ClientResult with core::KvEntry as the return type of GrpcClient::get_with_policy() and get_multi_with_policy(). Previously these power-user methods leaked a protobuf-generated type into the public API surface despite the rest of the read API (ClientApi::get, get_linearizable, etc.) already returning plain Bytes. Changes: - client_ext.rs: into_read_results() now returns Vec<Option<KvEntry>> - grpc_client.rs: get_with_policy / get_multi_with_policy return KvEntry - lib.rs: protocol module replaces ClientResult export with KvEntry - examples/client-usage-standalone: updated to KvEntry pattern match Tests (TDD -- red before green): - test_into_read_results_success: explicit Vec<Option<KvEntry>> annotation - test_into_read_results_multiple_entries_are_kv_entries (new) - test_get_with_policy_returns_native_kv_entry (new) - test_get_multi_with_policy_returns_native_kv_entries (new) proto::ClientResult survives only inside d-engine-proto's wire layer. All user-visible read results are now proto-free native Rust types.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds core, transport-agnostic client types; migrates client/server code, leader/state paths, state-machine reads, mocks, tests, examples, and public re-exports to use core types; introduces proto↔core conversion helpers and accompanying tests. ChangesClient Type Migration to Core
sequenceDiagram
participant Client
participant grpc_raft_service
participant proto_convert
participant leader_state
participant state_machine
Client->>grpc_raft_service: proto ClientRead/Write request
grpc_raft_service->>proto_convert: to_core_read_req / to_core_write_req
proto_convert->>leader_state: core ClientReadRequest/ClientWriteRequest
leader_state->>state_machine: apply/read
state_machine-->>leader_state: ClientResponse (core)
leader_state->>proto_convert: to_proto_response
proto_convert-->>grpc_raft_service: proto ClientResponse
grpc_raft_service-->>Client: gRPC response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
d-engine-core/src/state_machine_handler/mod.rs (1)
103-108:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve miss positions in multi-key reads.
Option<Vec<KvEntry>>cannot encode mixed hit/miss results in request order, so a read like[missing_key, present_key]gets collapsed and cannot satisfy the newVec<Option<KvEntry>>client contract. The read-path contract here needs one slot per requested key, e.g.Vec<Option<KvEntry>>, and implementations should fill it in request order instead of dropping misses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@d-engine-core/src/state_machine_handler/mod.rs` around lines 103 - 108, Change the read_from_state_machine signature and behavior to return a per-request-slot result so misses are preserved: update the fn read_from_state_machine(&self, keys: Vec<bytes::Bytes>) -> Vec<Option<KvEntry>> (replace the current Option<Vec<KvEntry>> return) and update all implementations and callers to construct and return a Vec<Option<KvEntry>> with one element per input key in the same request order (fill Some(KvEntry) for hits and None for misses) rather than dropping misses; ensure any code that consumed the old Option<Vec<KvEntry>> is adjusted to handle the new Vec<Option<KvEntry>> contract.d-engine-client/src/grpc_client_test.rs (1)
441-446:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winModel missing keys by omission, not empty values.
Line 445 currently injects an entry for a “missing” key using empty bytes, which tests empty-value semantics instead of missing-key semantics (
None).Suggested test fix
- for (i, key) in keys.iter().enumerate() { - client_results.push(KvEntry { - key: key.clone(), - value: match &values[i] { - Some(value) => value.clone(), - None => Bytes::copy_from_slice(&[]), // empty value for not found - }, - }); - } + for (i, key) in keys.iter().enumerate() { + if let Some(value) = &values[i] { + client_results.push(KvEntry { + key: key.clone(), + value: value.clone(), + }); + } + } @@ - assert_eq!( - results[i].as_ref().map(|r| r.value.clone()), - values[i].clone().or(Some(Bytes::new())) - ); + assert_eq!(results[i].as_ref().map(|r| r.value.clone()), values[i].clone());Also applies to: 488-490
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@d-engine-client/src/grpc_client_test.rs` around lines 441 - 446, The test currently models missing keys by inserting a KvEntry with an empty Bytes value; instead, when values[i] is None you should omit creating/pushing a KvEntry to represent a missing key. Update the match around client_results.push(KvEntry { key: key.clone(), value: ... }) so that Some(value) pushes a KvEntry with value.clone(), and None does not push anything (skip/continue). Apply the same change to the other occurrence that mirrors this logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@d-engine-core/src/client/types_test.rs`:
- Around line 7-39: The test test_error_code_all_variants_are_exhaustive
currently only asserts the array length which is not compile-time exhaustive;
replace the length-based check with a wildcard-free match over ErrorCode so the
compiler forces you to list every variant. Concretely, in
test_error_code_all_variants_are_exhaustive remove the assert_eq!(codes.len(),
23) and instead implement a function or expression that performs a match without
a `_` arm (e.g., fn assert_exhaustive(code: ErrorCode) { match code {
ErrorCode::Success => (), ErrorCode::ConnectionTimeout => (), ... } } ) and
invoke it in the test (listing every ErrorCode variant as explicit arms) so
adding a new variant will cause a compile error until the test is updated.
In `@d-engine-server/src/api/embedded_client.rs`:
- Around line 135-149: map_error_response currently discards server-provided
backoff guidance (response.retry_after_ms) and instead uses a fixed 100ms in
not_leader_error; update map_error_response so it extracts and forwards the
retry_after_ms from the server response (when present) into the returned
ClientApiError (pass it into not_leader_error or include it in server_error),
ensuring ErrorCode::NotLeader and the default branch preserve retry_after_ms;
search for map_error_response, not_leader_error, ClientApiError, ErrorCode,
LeaderHint and update their callsites listed in the comment to propagate
retry_after_ms instead of dropping it.
- Around line 297-302: The match on response.result currently treats any
non-Read ClientResponsePayload as a benign "not found" by using `_ => Ok(None)`;
instead, detect and surface protocol violations by returning an Err when
response.result is Some(...) but not ClientResponsePayload::Read. Update the
match handling around response.result (the branch that currently matches
ClientResponsePayload::Read and the `_ => Ok(None)` fallback) to return a
descriptive error (using the function's Result error type) for unexpected
payload variants rather than coercing them to None; apply the same change to the
other analogous block handling response.result (the block in the 373-384 region)
so malformed success payloads are surfaced consistently.
In `@d-engine-server/src/network/grpc/grpc_raft_service.rs`:
- Around line 415-418: The request validation only checks
proto_req.command.is_none() but doesn't verify that the nested
WriteCommand.operation is present, which can lead to a panic inside
proto_convert::to_core_write_req; update the validation before calling
to_core_write_req to return Err(Status::invalid_argument(...)) when either
proto_req.command is None or proto_req.command.as_ref().and_then(|c|
c.operation.as_ref()) is None (i.e., when a WriteCommand has operation == None),
so the method validates both proto_req and the nested WriteCommand.operation and
only then calls proto_convert::to_core_write_req.
- Around line 457-460: The conversion silently defaults invalid
consistency_policy values to None; modify the handling in the Read RPC path by
first validating the raw enum value from the incoming request (use the proto
enum's from_i32 / TryFrom) before calling proto_convert::to_core_read_req, and
if the value is invalid return a gRPC invalid-argument error instead of
proceeding; update the flow around proto_convert::to_core_read_req and the
self.cmd_tx.send(d_engine_core::ClientCmd::Read(core_req, resp_tx)) invocation
so only validated requests are forwarded (use an explicit mapping/validation
function for consistency_policy and return an error response when mapping
fails).
---
Outside diff comments:
In `@d-engine-client/src/grpc_client_test.rs`:
- Around line 441-446: The test currently models missing keys by inserting a
KvEntry with an empty Bytes value; instead, when values[i] is None you should
omit creating/pushing a KvEntry to represent a missing key. Update the match
around client_results.push(KvEntry { key: key.clone(), value: ... }) so that
Some(value) pushes a KvEntry with value.clone(), and None does not push anything
(skip/continue). Apply the same change to the other occurrence that mirrors this
logic.
In `@d-engine-core/src/state_machine_handler/mod.rs`:
- Around line 103-108: Change the read_from_state_machine signature and behavior
to return a per-request-slot result so misses are preserved: update the fn
read_from_state_machine(&self, keys: Vec<bytes::Bytes>) -> Vec<Option<KvEntry>>
(replace the current Option<Vec<KvEntry>> return) and update all implementations
and callers to construct and return a Vec<Option<KvEntry>> with one element per
input key in the same request order (fill Some(KvEntry) for hits and None for
misses) rather than dropping misses; ensure any code that consumed the old
Option<Vec<KvEntry>> is adjusted to handle the new Vec<Option<KvEntry>>
contract.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0da8f9f5-4e93-42a9-af44-c2c2b747dfea
📒 Files selected for processing (50)
d-engine-client/src/error_test.rsd-engine-client/src/grpc_client.rsd-engine-client/src/grpc_client_test.rsd-engine-client/src/lib.rsd-engine-client/src/mock_rpc.rsd-engine-client/src/mock_rpc_service.rsd-engine-client/src/pool.rsd-engine-client/src/pool_test.rsd-engine-client/src/proto/client_ext.rsd-engine-client/src/proto/client_ext_test.rsd-engine-core/src/client/client_api.rsd-engine-core/src/client/client_api_error.rsd-engine-core/src/client/client_api_error_test.rsd-engine-core/src/client/mod.rsd-engine-core/src/client/types.rsd-engine-core/src/client/types_test.rsd-engine-core/src/event.rsd-engine-core/src/raft_role/buffers/propose_batch_buffer.rsd-engine-core/src/raft_role/buffers/propose_batch_buffer_test.rsd-engine-core/src/raft_role/candidate_state_test.rsd-engine-core/src/raft_role/follower_state_test.rsd-engine-core/src/raft_role/leader_state.rsd-engine-core/src/raft_role/leader_state_test/backpressure_test.rsd-engine-core/src/raft_role/leader_state_test/buffer_cleanup_test.rsd-engine-core/src/raft_role/leader_state_test/client_read_test.rsd-engine-core/src/raft_role/leader_state_test/client_write_test.rsd-engine-core/src/raft_role/leader_state_test/commit_index_test.rsd-engine-core/src/raft_role/leader_state_test/deadline_test.rsd-engine-core/src/raft_role/leader_state_test/event_handling_test.rsd-engine-core/src/raft_role/leader_state_test/fatal_error_test.rsd-engine-core/src/raft_role/leader_state_test/pending_lease_reads_test.rsd-engine-core/src/raft_role/leader_state_test/pending_reads_test.rsd-engine-core/src/raft_role/leader_state_test/replication_test.rsd-engine-core/src/raft_role/learner_state_test.rsd-engine-core/src/raft_role/role_state.rsd-engine-core/src/replication/mod.rsd-engine-core/src/state_machine_handler/default_state_machine_handler.rsd-engine-core/src/state_machine_handler/mod.rsd-engine-server/src/api/embedded_client.rsd-engine-server/src/lib.rsd-engine-server/src/network/grpc/grpc_raft_service.rsd-engine-server/src/proto_convert.rsd-engine-server/src/proto_convert_test.rsd-engine-server/tests/cas_operations/leader_failover_cas_standalone.rsd-engine-server/tests/client_manager/mod.rsd-engine-server/tests/common/mod.rsd-engine-server/tests/embedded_client/embedded_client_operations.rsd-engine-server/tests/readonly_and_learner_mode/learner_readonly_sync_standalone.rsd-engine-server/tests/watch_and_subscriptions/watch_events_grpc_standalone.rsexamples/client-usage-standalone/src/main.rs
Comment on lines
+457
to
+460
| let core_req = proto_convert::to_core_read_req(request.into_inner()); | ||
| let (resp_tx, resp_rx) = MaybeCloneOneshot::new(); | ||
| self.cmd_tx | ||
| .send(d_engine_core::ClientCmd::Read( | ||
| request.into_inner(), | ||
| resp_tx, | ||
| )) | ||
| .send(d_engine_core::ClientCmd::Read(core_req, resp_tx)) |
There was a problem hiding this comment.
Reject invalid consistency_policy values instead of silently defaulting.
Line 457 converts the request without validating raw enum values first. Invalid integers currently degrade to None policy, which changes behavior silently.
Proposed fix
- let core_req = proto_convert::to_core_read_req(request.into_inner());
+ let proto_req = request.into_inner();
+ if let Some(raw) = proto_req.consistency_policy {
+ if d_engine_proto::client::ReadConsistencyPolicy::try_from(raw).is_err() {
+ return Err(Status::invalid_argument("Invalid consistency_policy"));
+ }
+ }
+ let core_req = proto_convert::to_core_read_req(proto_req);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@d-engine-server/src/network/grpc/grpc_raft_service.rs` around lines 457 -
460, The conversion silently defaults invalid consistency_policy values to None;
modify the handling in the Read RPC path by first validating the raw enum value
from the incoming request (use the proto enum's from_i32 / TryFrom) before
calling proto_convert::to_core_read_req, and if the value is invalid return a
gRPC invalid-argument error instead of proceeding; update the flow around
proto_convert::to_core_read_req and the
self.cmd_tx.send(d_engine_core::ClientCmd::Read(core_req, resp_tx)) invocation
so only validated requests are forwarded (use an explicit mapping/validation
function for consistency_policy and return an error response when mapping
fails).
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Issue 1 (Minor) — types_test.rs:
Add compile-time exhaustiveness guard for ErrorCode variants.
The previous length-based assertion could be bypassed by adding a
variant without updating the array; a wildcard-free match function
now guarantees a compile error if any variant is unhandled.
Issue 4 (Critical) — grpc_raft_service.rs:
Validate the nested WriteCommand.operation field at the gRPC boundary.
Previously only command.is_some() was checked; a request with
WriteCommand { operation: None } passed validation and triggered
an unreachable!() panic in proto_convert -- externally exploitable.
Now a single check covers both the outer command and inner operation.
Not addressed (pre-existing, separate tickets):
- embedded_client.rs: retry_after_ms dropped in map_error_response
- embedded_client.rs: non-read payloads silently coerced to None
- grpc_raft_service.rs: invalid consistency_policy int -- intentional
proto3 degradation to server default, not a bug
JoshuaChi
added a commit
that referenced
this pull request
May 22, 2026
…pected read payloads; warn on unknown consistency_policy
embedded_client: thread `retry_after_ms` from server responses through
`map_error_response` and `not_leader_error` (all 6 callsites). Falls
back to 100ms only when server provides None. Previously all NotLeader
errors silently discarded server-side backoff guidance.
embedded_client: extract `extract_read_payload` helper and replace
`_ => Ok(None)` / `_ => Ok(vec![None; n])` wildcard arms in
`get_with_consistency` and `get_multi_with_consistency`. A WriteResult
or missing payload in a read response now surfaces as
`Protocol { InvalidResponse }` instead of masking the violation as
"key not found".
grpc_raft_service: emit `warn!` when an unrecognised
`consistency_policy` integer is received and degraded to cluster
default. Preserves proto3 forward-compatibility while making
silent degradation observable in logs.
Tests: 9 unit tests in `error_helper_tests` —
- not_leader_error: server value forwarding, None fallback, Some(0) edge case
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
d-engine-server/src/api/embedded_client.rs (1)
289-293:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the example to import the new core enum.
The example above this method still uses
d_engine_proto::client::ReadConsistencyPolicy, but this API now takesd_engine_core::config::ReadConsistencyPolicy. Following the docs will fail for callers migrating to the new surface.📝 Proposed fix
- /// use d_engine_proto::client::ReadConsistencyPolicy; + /// use d_engine_core::config::ReadConsistencyPolicy;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@d-engine-server/src/api/embedded_client.rs` around lines 289 - 293, The example above the get_with_consistency method references the old enum path; update any imports and example usage to use d_engine_core::config::ReadConsistencyPolicy instead of d_engine_proto::client::ReadConsistencyPolicy so callers of get_with_consistency (the method signature taking ReadConsistencyPolicy) compile against the new core enum; locate examples around the get_with_consistency function and replace the import and any fully-qualified references to the old module with d_engine_core::config::ReadConsistencyPolicy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@d-engine-server/src/api/embedded_client.rs`:
- Around line 289-293: The example above the get_with_consistency method
references the old enum path; update any imports and example usage to use
d_engine_core::config::ReadConsistencyPolicy instead of
d_engine_proto::client::ReadConsistencyPolicy so callers of get_with_consistency
(the method signature taking ReadConsistencyPolicy) compile against the new core
enum; locate examples around the get_with_consistency function and replace the
import and any fully-qualified references to the old module with
d_engine_core::config::ReadConsistencyPolicy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 73dfe28d-8682-4e52-9323-056f1da9be2b
📒 Files selected for processing (2)
d-engine-server/src/api/embedded_client.rsd-engine-server/src/network/grpc/grpc_raft_service.rs
…pected read payloads; warn on unknown consistency_policy
JoshuaChi
force-pushed
the
refactor/260-d-engine-core-dep
branch
from
May 22, 2026 15:20
f508297 to
b41dfa1
Compare
Merged
10 of 12 tasks
12 of 17 tasks
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
1 participant
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.
What Does This PR Do?
Decouples d-engine-core's public API from d-engine-proto by defining
native Rust types (
ErrorCode,ClientResponse,KvEntry,LeaderHint,ReadConsistencyPolicy) in core and confining all proto↔core conversionsto a single layer (
d-engine-server/src/proto_convert.rs). Users nowprogram against stable, semantically meaningful Rust types rather than
protobuf-generated code.
Type:
Why Is This Needed?
Issue: #260
Previously,
ErrorCode,ReadConsistencyPolicy, andClientResultwerere-exported directly from
d-engine-protointo the public API. Thismeant users had to import auto-generated protobuf types, and any
wire-format change (field rename, enum reorder) could silently break
application code. It also made it impossible to add an HTTP/QUIC
transport without touching
d-engine-core.This PR draws a hard boundary:
Checklist
Required:
make testpasses (formatting + Clippy-D warnings+ 1 375 tests)If changing APIs:
comments)
header documents the orphan-rule rationale and performance notes)
Testing
How tested:
d-engine-core/src/client/types_test.rs— 18 newtests covering
ClientResponseconstructors and predicates(
is_write_success,is_term_outdated,is_propose_failure,not_leaderwith/without hint,TryFrom<i32> for ErrorCode— all 23discriminants)
d-engine-server/src/proto_convert_test.rs— round-triptests for every conversion function in the new conversion layer
client_ext_test.rs—into_read_resultsnowexplicitly type-annotated
Vec<Option<KvEntry>>; addedtest_into_read_results_multiple_entries_are_kv_entriesgrpc_client_test.rs— addedtest_get_with_policy_returns_native_kv_entryandtest_get_multi_with_policy_returns_native_kv_entrieswith explicitOption<KvEntry>/Vec<Option<KvEntry>>type annotations (thesefail to compile until the return type is changed — true red/green cycle)
d-engine-server/tests/pass unchangedFor bug fixes: N/A
Does This Follow d-engine's Principles?
any user importing
ErrorCodeorReadConsistencyPolicywasdepending on a generated type; this removes that coupling for everyone
#[inline]freefunctions in one file; no new traits, no new crates
ClientResultandproto
ErrorCode/ReadConsistencyPolicyremoved from publicexports; replaced by equivalents already in
d-engine-coreReviewer Notes
Focus areas:
d-engine-server/src/proto_convert.rs— the new conversion layer.Verify
core_error_to_proto/proto_error_to_coreare symmetricand that the
TryFrom<i32>discriminant list ind-engine-core/src/client/types.rsstays in sync witherror.proto.proto_convert.rshas three#[allow(dead_code)]functions(
proto_error_to_core,to_core_response,parse_leader_hint).These are the reverse path (proto → core for client-side parsing) and
are intentionally kept for future use; ticket perf(read): linearizable read lease fast path + fix Raft lease clock (SystemTime → Instant) #390 tracks whether to
promote or remove them.
GrpcClient::get_with_policyandget_multi_with_policyare the onlypublic methods that previously returned a proto type directly. They
now return
Option<KvEntry>/Vec<Option<KvEntry>>. This is abreaking change for any caller that pattern-matched on
proto::ClientResult— the example inexamples/client-usage-standaloneshows the migration.Estimated review complexity:
the entire workspace; the logical change is narrow (type substitution
Summary by CodeRabbit
New Features
API Changes
Tests