diff --git a/crates/config/src/constants/internode.rs b/crates/config/src/constants/internode.rs index bddb8c286..8814b8b3e 100644 --- a/crates/config/src/constants/internode.rs +++ b/crates/config/src/constants/internode.rs @@ -97,19 +97,26 @@ pub const ENV_INTERNODE_RPC_MAX_MESSAGE_SIZE: &str = "RUSTFS_INTERNODE_RPC_MAX_M pub const ENV_INTERNODE_RPC_LARGE_PAYLOAD_WARN_BYTES: &str = "RUSTFS_INTERNODE_RPC_LARGE_PAYLOAD_WARN_BYTES"; pub const DEFAULT_INTERNODE_RPC_LARGE_PAYLOAD_WARN_BYTES: usize = 8 * 1024 * 1024; -/// Stop dual-writing the JSON compatibility strings on internode metadata RPCs and send only the +/// Request stopping the JSON compatibility strings on internode metadata RPCs and sending only the /// msgpack `_bin` payloads (grpc-optimization P2-1). /// -/// Defaults to `false` (dual-write, byte-for-byte legacy behavior). This is a rollout lever, not a -/// wire-format change: it may only be enabled **after** the JSON-fallback counter -/// (`rustfs_system_network_internode_msgpack_json_fallback_total`) has read zero across a release -/// window fleet-wide, confirming every peer decodes `_bin` first. Single-env rollback. See +/// Defaults to `false` (dual-write, byte-for-byte legacy behavior). This is only a request; RustFS +/// keeps JSON compatibility fields unless [`ENV_INTERNODE_RPC_MSGPACK_ONLY_FLEET_CONFIRMED`] is also +/// true after the release-window convergence and rollback gates pass. See /// `docs/operations/internode-msgpack-json-convergence-runbook.md`. pub const ENV_INTERNODE_RPC_MSGPACK_ONLY: &str = "RUSTFS_INTERNODE_RPC_MSGPACK_ONLY"; pub const DEFAULT_INTERNODE_RPC_MSGPACK_ONLY: bool = false; -// Compile-time invariant: dual-write by default so the base build is byte-for-byte legacy behavior. +/// Explicit fleet-wide confirmation gate for [`ENV_INTERNODE_RPC_MSGPACK_ONLY`]. +/// +/// This separate default-off guard prevents a single legacy flag from accidentally emptying JSON +/// fields in a mixed-version fleet where an older peer still reads the JSON field. +pub const ENV_INTERNODE_RPC_MSGPACK_ONLY_FLEET_CONFIRMED: &str = "RUSTFS_INTERNODE_RPC_MSGPACK_ONLY_FLEET_CONFIRMED"; +pub const DEFAULT_INTERNODE_RPC_MSGPACK_ONLY_FLEET_CONFIRMED: bool = false; + +// Compile-time invariants: dual-write by default so the base build is byte-for-byte legacy behavior. const _: () = assert!(!DEFAULT_INTERNODE_RPC_MSGPACK_ONLY); +const _: () = assert!(!DEFAULT_INTERNODE_RPC_MSGPACK_ONLY_FLEET_CONFIRMED); /// Consecutive-failure threshold after which an internode peer is marked offline (grpc-optimization /// P3 observability). @@ -273,8 +280,12 @@ mod tests { #[test] fn internode_msgpack_only_env_name_is_stable() { - // The dual-write-by-default invariant is asserted at compile time next to the definition. + // The dual-write-by-default invariants are asserted at compile time next to the definitions. assert_eq!(ENV_INTERNODE_RPC_MSGPACK_ONLY, "RUSTFS_INTERNODE_RPC_MSGPACK_ONLY"); + assert_eq!( + ENV_INTERNODE_RPC_MSGPACK_ONLY_FLEET_CONFIRMED, + "RUSTFS_INTERNODE_RPC_MSGPACK_ONLY_FLEET_CONFIRMED" + ); } #[test] diff --git a/crates/ecstore/src/cluster/rpc/remote_disk.rs b/crates/ecstore/src/cluster/rpc/remote_disk.rs index 6e4f1613a..cd8b44f7c 100644 --- a/crates/ecstore/src/cluster/rpc/remote_disk.rs +++ b/crates/ecstore/src/cluster/rpc/remote_disk.rs @@ -1022,9 +1022,8 @@ fn encode_msgpack(value: &T) -> Result> { } /// JSON compatibility string for a dual-encoded (`_bin` + text) request field. Returns an empty -/// string when msgpack-only mode is enabled (grpc-optimization P2-1) so the redundant JSON copy is -/// not sent; otherwise the legacy JSON encoding. Only use for fields whose peer decodes `_bin` -/// first — the paired `_bin` (msgpack) field must always be sent alongside. +/// string only when msgpack-only mode and its explicit fleet confirmation guard are both enabled; +/// otherwise the legacy JSON encoding is retained for old peers. fn compat_json(value: &T) -> Result { if rustfs_protos::internode_rpc_msgpack_only() { return Ok(String::new()); @@ -3019,15 +3018,45 @@ mod tests { #[test] fn compat_json_dual_writes_by_default() { - // msgpack-only defaults off, so compat_json returns the JSON encoding (dual-write). The - // empty-string (msgpack-only) path is exercised via the env flag in integration, not here, - // to keep this test independent of process-global env state. let resp = sample_read_multiple_resp("file", b"data"); let json = compat_json(&resp).expect("compat_json should encode"); assert!(!json.is_empty()); assert_eq!(json, serde_json::to_string(&resp).expect("json should encode")); } + #[test] + fn compat_json_keeps_json_when_msgpack_only_lacks_fleet_confirmation() { + temp_env::with_vars( + [ + (rustfs_config::ENV_INTERNODE_RPC_MSGPACK_ONLY, Some("true")), + (rustfs_config::ENV_INTERNODE_RPC_MSGPACK_ONLY_FLEET_CONFIRMED, None::<&str>), + ], + || { + let resp = sample_read_multiple_resp("file", b"data"); + let json = compat_json(&resp).expect("compat_json should encode"); + + assert!(!json.is_empty(), "old JSON peers must remain compatible without fleet confirmation"); + assert_eq!(json, serde_json::to_string(&resp).expect("json should encode")); + }, + ); + } + + #[test] + fn compat_json_omits_json_only_after_fleet_confirmation() { + temp_env::with_vars( + [ + (rustfs_config::ENV_INTERNODE_RPC_MSGPACK_ONLY, Some("true")), + (rustfs_config::ENV_INTERNODE_RPC_MSGPACK_ONLY_FLEET_CONFIRMED, Some("true")), + ], + || { + let resp = sample_read_multiple_resp("file", b"data"); + let json = compat_json(&resp).expect("compat_json should encode"); + + assert!(json.is_empty(), "msgpack-only may empty JSON only after explicit fleet confirmation"); + }, + ); + } + #[test] fn read_multiple_response_decode_reports_corrupt_msgpack_item() { let endpoint = sample_remote_endpoint(); diff --git a/crates/protos/src/lib.rs b/crates/protos/src/lib.rs index fc34ae66e..d10dd066c 100644 --- a/crates/protos/src/lib.rs +++ b/crates/protos/src/lib.rs @@ -636,12 +636,18 @@ mod tier_mutation_rpc_tests { /// Whether internode metadata RPCs should send only the msgpack `_bin` payloads and leave the JSON /// compatibility strings empty (grpc-optimization P2-1). Shared by the client (`remote_disk`) and -/// server (`node_service`) send paths. Defaults to `false` (dual-write); see -/// [`rustfs_config::ENV_INTERNODE_RPC_MSGPACK_ONLY`] and the convergence runbook before enabling. +/// server (`node_service`) send paths. +/// +/// The legacy flag alone is deliberately insufficient: emptying JSON breaks old peers that only +/// decode the compatibility field. Operators must also set the fleet-confirmed guard after the +/// convergence runbook proves every peer supports `_bin` and rollback. pub fn internode_rpc_msgpack_only() -> bool { rustfs_utils::get_env_bool( rustfs_config::ENV_INTERNODE_RPC_MSGPACK_ONLY, rustfs_config::DEFAULT_INTERNODE_RPC_MSGPACK_ONLY, + ) && rustfs_utils::get_env_bool( + rustfs_config::ENV_INTERNODE_RPC_MSGPACK_ONLY_FLEET_CONFIRMED, + rustfs_config::DEFAULT_INTERNODE_RPC_MSGPACK_ONLY_FLEET_CONFIRMED, ) } diff --git a/docs/operations/internode-msgpack-json-convergence-runbook.md b/docs/operations/internode-msgpack-json-convergence-runbook.md index 268f6da73..009b612f3 100644 --- a/docs/operations/internode-msgpack-json-convergence-runbook.md +++ b/docs/operations/internode-msgpack-json-convergence-runbook.md @@ -106,18 +106,18 @@ fallback counter has read zero across a window with the new decoders fully deplo ## Stage 1 — Stop writing JSON (env-gated, after Stage 0 reads zero) -The send-side lever is **implemented** and gated by the default-off env flag -`RUSTFS_INTERNODE_RPC_MSGPACK_ONLY`. When enabled, the convergence-ready fields above send -only `_bin` and leave the JSON string empty; the `_bin` payload is always sent and decoders -keep the JSON read fallback unchanged. The delete fields are excluded (dual-write) per the -section above. +The send-side lever is **implemented** and requires two default-off env flags: + +- `RUSTFS_INTERNODE_RPC_MSGPACK_ONLY=true` +- `RUSTFS_INTERNODE_RPC_MSGPACK_ONLY_FLEET_CONFIRMED=true` + +The first flag only requests msgpack-only. The second flag is the explicit proof gate that the fleet has passed the release-window fallback, capability, and rollback checks. If only `RUSTFS_INTERNODE_RPC_MSGPACK_ONLY=true` is set, RustFS keeps dual-writing JSON compatibility fields so old JSON-only peers remain compatible. When both flags are enabled, the convergence-ready fields above send only `_bin` and leave the JSON string empty; the `_bin` payload is always sent and decoders keep the JSON read fallback unchanged. The delete fields are excluded (dual-write) per the section above. Only enable it **after** Stage 0 has read zero for a full window across the fleet: 1. Ship with the flag **off** (no behavior change). -2. Enable it on one node (`RUSTFS_INTERNODE_RPC_MSGPACK_ONLY=true`, restart) and watch the - fallback counter for a soak period. If it stays zero, enable fleet-wide. -3. **Rollback:** set `RUSTFS_INTERNODE_RPC_MSGPACK_ONLY=false` (or unset) and restart. No +2. Enable it on one node with both `RUSTFS_INTERNODE_RPC_MSGPACK_ONLY=true` and `RUSTFS_INTERNODE_RPC_MSGPACK_ONLY_FLEET_CONFIRMED=true`, then restart and watch the fallback counter for a soak period. If it stays zero, enable fleet-wide. +3. **Rollback:** set either `RUSTFS_INTERNODE_RPC_MSGPACK_ONLY=false` or `RUSTFS_INTERNODE_RPC_MSGPACK_ONLY_FLEET_CONFIRMED=false` (or unset either flag) and restart. No wire-format was broken in this stage, so rollback is immediate and safe. ## Stage 2 — Remove the proto JSON fields (next release, N+1) @@ -135,7 +135,7 @@ still zero. | Stage | Wire-format broken? | Rollback | |---|---|---| | 0 Observe | no | n/a (metric only) | -| 1 msgpack-only send | no | `RUSTFS_INTERNODE_RPC_MSGPACK_ONLY=false` + restart | +| 1 msgpack-only send | no | unset either msgpack-only env flag + restart | | 2 remove fields | yes | redeploy prior release; field numbers stay `reserved` | ## Related diff --git a/rustfs/src/storage/rpc/node_service/disk.rs b/rustfs/src/storage/rpc/node_service/disk.rs index 9c640c481..10292c9c3 100644 --- a/rustfs/src/storage/rpc/node_service/disk.rs +++ b/rustfs/src/storage/rpc/node_service/disk.rs @@ -67,9 +67,9 @@ fn encode_msgpack_named(value: &T, value_name: &str) -> std Ok(serializer.into_inner()) } -/// JSON compatibility string for a dual-encoded response field. Returns an empty string when -/// msgpack-only mode is enabled (grpc-optimization P2-1) so the redundant JSON copy is not sent; -/// otherwise the legacy JSON encoding. The paired `_bin` (msgpack) field is always sent. +/// JSON compatibility string for a dual-encoded response field. Returns an empty string only when +/// msgpack-only mode and its explicit fleet confirmation guard are both enabled; otherwise the +/// legacy JSON encoding is retained for old peers. The paired `_bin` field is always sent. fn compat_response_json(value: &T) -> std::result::Result { if rustfs_protos::internode_rpc_msgpack_only() { return Ok(String::new()); @@ -1175,8 +1175,8 @@ impl NodeService { #[cfg(test)] mod tests { use super::{ - decode_msgpack_or_json, encode_batch_read_version_response_payloads, encode_msgpack, - encode_read_multiple_response_payloads, + compat_response_json, decode_msgpack_or_json, encode_batch_read_version_response_payloads, encode_msgpack, + encode_msgpack_named, encode_read_multiple_response_payloads, }; use crate::storage::storage_api::ReadMultipleResp; use crate::storage::storage_api::rpc_consumer::node_service::BatchReadVersionResp; @@ -1215,6 +1215,75 @@ mod tests { ); } + #[test] + fn compat_response_json_keeps_json_when_msgpack_only_lacks_fleet_confirmation() { + temp_env::with_vars( + [ + (rustfs_config::ENV_INTERNODE_RPC_MSGPACK_ONLY, Some("true")), + (rustfs_config::ENV_INTERNODE_RPC_MSGPACK_ONLY_FLEET_CONFIRMED, None::<&str>), + ], + || { + let payload = SamplePayload { + name: "legacy-json".to_string(), + count: 9, + }; + let json = compat_response_json(&payload).expect("compat response json should encode"); + + assert!(!json.is_empty(), "old JSON clients must remain compatible without fleet confirmation"); + assert_eq!(json, serde_json::to_string(&payload).expect("json should encode")); + }, + ); + } + + #[test] + fn compat_response_json_omits_json_only_after_fleet_confirmation() { + temp_env::with_vars( + [ + (rustfs_config::ENV_INTERNODE_RPC_MSGPACK_ONLY, Some("true")), + (rustfs_config::ENV_INTERNODE_RPC_MSGPACK_ONLY_FLEET_CONFIRMED, Some("true")), + ], + || { + let payload = SamplePayload { + name: "msgpack-only".to_string(), + count: 10, + }; + let json = compat_response_json(&payload).expect("compat response json should encode"); + + assert!(json.is_empty(), "msgpack-only may empty JSON only after explicit fleet confirmation"); + }, + ); + } + + #[test] + fn decode_msgpack_or_json_fails_closed_on_corrupt_non_empty_msgpack() { + let err = decode_msgpack_or_json::(b"not-msgpack", r#"{"name":"json","count":1}"#, "SamplePayload") + .expect_err("corrupt non-empty msgpack must not fall back to JSON"); + + assert!(err.to_string().contains("decode SamplePayload msgpack failed"), "unexpected error: {err}"); + } + + #[test] + fn decode_msgpack_or_json_accepts_named_msgpack_and_legacy_json() { + let payload = SamplePayload { + name: "named".to_string(), + count: 11, + }; + let named_msgpack = encode_msgpack_named(&payload, "SamplePayload").expect("named msgpack should encode"); + let decoded_msgpack = + decode_msgpack_or_json::(&named_msgpack, "", "SamplePayload").expect("named msgpack should decode"); + let decoded_json = decode_msgpack_or_json::(&[], r#"{"name":"legacy-json","count":12}"#, "SamplePayload") + .expect("legacy json should decode"); + + assert_eq!(decoded_msgpack, payload); + assert_eq!( + decoded_json, + SamplePayload { + name: "legacy-json".to_string(), + count: 12 + } + ); + } + #[test] fn encode_read_multiple_response_payloads_keeps_json_and_msgpack_in_sync() { let responses = vec![