This is an automated email from the ASF dual-hosted git repository. hubcio pushed a commit to branch feat/command-extensibility in repository https://gitbox.apache.org/repos/asf/iggy.git
commit 05730ace2b5e512effe3c1a31fb31ecb959fd82f Author: Hubert Gruszecki <[email protected]> AuthorDate: Tue Jul 28 14:21:31 2026 +0200 fix(sdk): forward unknown command codes instead of rejecting them An application built on this SDK cannot reach a command its SDK build has never heard of. operation_for_code ended None => InvalidCommand, so a code absent from COMMAND_TABLE failed at encode time, before any byte left the process, and a server that does implement it never got the chance to answer. COMMAND_TABLE is a protocol registry, not a per-server capability list, so the server is the authority on whether a code exists: an unknown code now ships as non-replicated, carrying the code in RequestHeader.reserved, and the server answers with a proper error if it does not know it. The SDK no longer second-guesses that classification either. It used to refuse a code the registry knows as replicated, but the server re-derives the class from the same registry and denies a mismatch on a path that commits nothing, so the client check duplicated a guard it does not own. It was unreachable besides, since every replicated entry resolves through from_command_code first, and it was the sole construction of an UnknownReplicatedCommand variant nothing in the tree consumed. operation_for_code is now infallible, the raw-command documentation no longer explains consensus to callers, and error code 14002 is retired rather than reused. --- core/common/src/error/iggy_error.rs | 2 -- core/integration/tests/sdk/raw.rs | 13 +++++-- core/sdk/src/clients/client.rs | 5 ++- core/sdk/src/vsr.rs | 69 ++++++++++++++++++++++++++++++------- 4 files changed, 70 insertions(+), 19 deletions(-) diff --git a/core/common/src/error/iggy_error.rs b/core/common/src/error/iggy_error.rs index 878704e0a..8d2edc9eb 100644 --- a/core/common/src/error/iggy_error.rs +++ b/core/common/src/error/iggy_error.rs @@ -522,8 +522,6 @@ pub enum IggyError { AlreadyAuthenticated = 14000, #[error("VSR session value {0} is invalid (must be non-zero)")] InvalidSession(u64) = 14001, - #[error("Replicated command with unknown code {0}")] - UnknownReplicatedCommand(u32) = 14002, /// Packed protocol versions, see `iggy_binary_protocol::ProtocolVersion`. /// Field order: `(client_version, server_min, server_max)`. #[error( diff --git a/core/integration/tests/sdk/raw.rs b/core/integration/tests/sdk/raw.rs index cffeec954..0bf004947 100644 --- a/core/integration/tests/sdk/raw.rs +++ b/core/integration/tests/sdk/raw.rs @@ -87,14 +87,23 @@ async fn assert_raw_round_trip(client: &IggyClient) { .expect_err("session-control codes must be rejected by the raw path"); assert_eq!(error, IggyError::InvalidCommand); - // VSR encoder is closed-world: unknown code rejected at encode time. + // An SDK build cannot know which codes a server implements, so an + // unknown one ships as non-replicated and the server decides. The + // follow-up ping is the point of the case: the server must answer + // with a deny frame, not drop the frame and leave the connection + // wedged until the read timeout. #[cfg(feature = "vsr")] { let error = client .send_binary_request(60_000, Bytes::new()) .await - .expect_err("unknown code must be rejected under VSR"); + .expect_err("unknown code must be refused by the server"); assert_eq!(error, IggyError::InvalidCommand); + + client + .send_binary_request(PING_CODE, PingRequest.to_bytes()) + .await + .expect("connection must survive an unknown code"); } let error = client diff --git a/core/sdk/src/clients/client.rs b/core/sdk/src/clients/client.rs index 98beb9830..dd7f91fd9 100644 --- a/core/sdk/src/clients/client.rs +++ b/core/sdk/src/clients/client.rs @@ -204,9 +204,8 @@ impl IggyClient { /// Login and logout codes are rejected with `InvalidCommand`. Use the /// `login_user` / `logout_user` methods so SDK session state stays correct. /// - /// Custom codes only work on the classic protocol. Under `vsr` the encoder - /// is closed-world: an unknown code yields `InvalidCommand`, a replicated - /// code with no mapping yields `UnknownReplicatedCommand`. + /// Custom codes are forwarded to the server, which is the authority on + /// whether it implements them. pub async fn send_binary_request(&self, code: u32, payload: Bytes) -> Result<Bytes, IggyError> { if SESSION_CONTROL_CODES.contains(&code) { return Err(IggyError::InvalidCommand); diff --git a/core/sdk/src/vsr.rs b/core/sdk/src/vsr.rs index 90a10a552..9bc5d1cd1 100644 --- a/core/sdk/src/vsr.rs +++ b/core/sdk/src/vsr.rs @@ -88,7 +88,7 @@ pub(crate) fn encode_request_header( (Operation::Register, session.begin_register(), 0) } _ => { - let operation = operation_for_code(code)?; + let operation = operation_for_code(code); // NonReplicated ops (ping, reads) bypass server-side dedup -- // `ClientTable` only tracks request_ids for replicated ops. If // they consumed the monotonic counter, the next replicated @@ -153,20 +153,17 @@ pub(crate) fn encode_request_header( Ok((header, total_size)) } -fn operation_for_code(code: u32) -> Result<Operation, IggyError> { +/// `COMMAND_TABLE` is a protocol registry, not a per-server capability list, so +/// an SDK build cannot know which codes a given server implements. The server is +/// the authority: an unmapped code is forwarded as non-replicated (the code +/// rides `RequestHeader.reserved`, which that path already stamps) and the +/// server answers with a proper error if it does not know it. +fn operation_for_code(code: u32) -> Operation { if code == LOGOUT_USER_CODE { - return Ok(Operation::Logout); + return Operation::Logout; } - if let Some(operation) = Operation::from_command_code(code) { - return Ok(operation); - } - - match iggy_binary_protocol::dispatch::lookup_command(code) { - Some(meta) if !meta.is_replicated() => Ok(Operation::NonReplicated), - Some(_) => Err(IggyError::UnknownReplicatedCommand(code)), - None => Err(IggyError::InvalidCommand), - } + Operation::from_command_code(code).unwrap_or(Operation::NonReplicated) } pub(crate) fn response_size(header: &[u8]) -> Result<usize, IggyError> { @@ -692,6 +689,54 @@ mod tests { assert_eq!(header.session, 99); } + #[test] + fn unknown_code_encodes_as_non_replicated_and_carries_the_code() { + // An extended server may implement codes this SDK build has never heard + // of. The registry is not a capability list, so the request must reach + // the server rather than fail at encode time. + const UNKNOWN_CODE: u32 = 60_000; + assert!( + iggy_binary_protocol::dispatch::lookup_command(UNKNOWN_CODE).is_none(), + "test needs a code absent from COMMAND_TABLE" + ); + + let mut session = ConsensusSession::with_client_id(42); + session.bind(99); + let bytes = encode_contiguous_request(&mut session, UNKNOWN_CODE, &Bytes::new()).unwrap(); + let header = decode_request_header(&bytes); + + assert_eq!(header.operation, Operation::NonReplicated); + assert_eq!( + u32::from_le_bytes( + header.reserved[NON_REPLICATED_CODE_RANGE] + .try_into() + .unwrap() + ), + UNKNOWN_CODE + ); + } + + #[test] + fn no_replicated_command_ever_resolves_to_non_replicated() { + // The safety asymmetry that makes forwarding unknown codes acceptable: + // an unknown code is the server's business, but a *known* replicated + // command sent as non-replicated would apply on one node only and + // silently diverge the replicas. Swept over the whole registry so a + // future entry cannot regress it. + for meta in iggy_binary_protocol::dispatch::COMMAND_TABLE { + if !meta.is_replicated() { + continue; + } + assert_ne!( + operation_for_code(meta.code), + Operation::NonReplicated, + "replicated command {} ({}) must never encode as NonReplicated", + meta.name, + meta.code + ); + } + } + #[test] fn namespace_defers_named_identifiers_to_server_resolution() { let stream = WireIdentifier::named("stream").unwrap();
