fix(ipc): handle version mismatches gracefully without confusing bincode errors#213
Closed
paperbenni wants to merge 3 commits intomainfrom
Closed
fix(ipc): handle version mismatches gracefully without confusing bincode errors#213paperbenni wants to merge 3 commits intomainfrom
paperbenni wants to merge 3 commits intomainfrom
Conversation
When the `instantwmctl` client and the `instantwm` server were built from different commits (resulting in different struct/enum layouts), `bincode` would fail during deserialization with a generic `UnexpectedVariant` error. This commit implements two fixes: 1. **Protocol Versioning**: Appends the first 8 characters of the current git commit hash to the `IPC_PROTOCOL_VERSION` generated in `build.rs`. This ensures different builds uniquely identify themselves. 2. **Graceful Deserialization**: In the server IPC handler, we now partially decode just the version `String` from the byte buffer *before* attempting to deserialize the entire `IpcRequest` struct. If a mismatch is detected, we can immediately return a friendly error message instead of letting bincode crash. 3. **Friendly Errors**: The client-side (`instantwmctl.rs`) now provides a clearer error message indicating a version mismatch when deserialization fails. Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideImproves IPC protocol compatibility handling by strengthening version identification and adding graceful error handling and messaging when client/server versions of instantwm and instantwmctl do not match. Sequence diagram for IPC request handling with graceful version mismatchsequenceDiagram
actor User
participant Instantwmctl as instantwmctl
participant IpcServer as IpcServer
User->>Instantwmctl: Run command
Instantwmctl->>IpcServer: Send IPC request bytes
Note over IpcServer: New behavior: decode version string first
IpcServer->>IpcServer: bincode_decode_from_slice_String(buffer)
alt Version_decoded_and_mismatch
IpcServer->>Instantwmctl: IpcResponse_err("version mismatch: client is X, server is Y. Please ensure instantwmctl and instantWM are the same version.")
Instantwmctl->>User: Print error message
else Version_decode_fails
IpcServer->>IpcServer: Treat as mismatch_or_garbage
IpcServer->>IpcServer: Continue to full request decode
IpcServer->>IpcServer: bincode_decode_from_slice_IpcRequest(buffer)
alt Full_decode_ok_and_version_valid
IpcServer->>IpcServer: request_validate_version()
IpcServer-->>Instantwmctl: Normal response
else Full_decode_error
IpcServer-->>Instantwmctl: IpcResponse_err("deserialize error (likely version mismatch): e")
end
else Version_matches
IpcServer->>IpcServer: bincode_decode_from_slice_IpcRequest(buffer)
alt Full_decode_ok_and_version_valid
IpcServer->>IpcServer: request_validate_version()
IpcServer-->>Instantwmctl: Normal response
else Full_decode_error
IpcServer-->>Instantwmctl: IpcResponse_err("deserialize error (likely version mismatch): e")
end
end
Instantwmctl->>Instantwmctl: Deserialize response
alt Deserialize_success
Instantwmctl->>User: Show command result
else Deserialize_failure
Instantwmctl->>User: Print "instantwmctl: deserialization failed: e"
Instantwmctl->>User: Print "This is likely caused by a version mismatch between instantwmctl and instantWM."
Instantwmctl->>User: Print "Please ensure they are built from the same commit."
Instantwmctl->>Instantwmctl: Exit 1
end
Flow diagram for IPC protocol version generation in build.rsflowchart TD
A[Start build_rs main] --> B[Read CARGO_PKG_VERSION as crate_version]
B --> C[compute_ipc_source_hash to get source_hash]
C --> D[git_head_commit to get build_commit or fallback to unknown]
D --> E[Take first 8 chars of source_hash]
E --> F[Take first 8 chars of build_commit]
F --> G[Format protocol_version as crate_version-source_hash8-build_commit8]
G --> H[Print cargo:rustc-env=IPC_PROTOCOL_VERSION=protocol_version]
D --> I[Print cargo:rustc-env=INSTANTWM_BUILD_COMMIT=build_commit]
H --> J[End]
I --> J[End]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (24)
📝 WalkthroughWalkthroughThis PR introduces urgent window/tag state support with dedicated color schemes, restructures the IPC module from a monolithic file into domain-specific submodules (general, input, keyboard, mode, monitor, scratchpad, tag, toggle, window), adds Git commit hash tracking to the protocol version, and implements various logic adjustments across animation, cursor handling, and keyboard state management. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Member
Author
|
@jules ipc.rs has since been refactored, resolve the merge conflicts |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The IPC handler now validates the protocol version twice (manual string decode plus
request.validate_version()); consider centralizing this check in one place to avoid divergence in future changes. - When the initial
Stringdecode for the version fails, the code silently proceeds to full deserialization; it might be helpful to log or at least respond with a clearer error in that branch to distinguish truly malformed input from other issues. - Including
"unknown"as the git commit in the protocol version couples compatibility to a potentially meaningless tag; you may want to omit the commit fromIPC_PROTOCOL_VERSIONwhen it cannot be determined, to avoid spurious version mismatches between identical builds without git metadata.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The IPC handler now validates the protocol version twice (manual string decode plus `request.validate_version()`); consider centralizing this check in one place to avoid divergence in future changes.
- When the initial `String` decode for the version fails, the code silently proceeds to full deserialization; it might be helpful to log or at least respond with a clearer error in that branch to distinguish truly malformed input from other issues.
- Including `"unknown"` as the git commit in the protocol version couples compatibility to a potentially meaningless tag; you may want to omit the commit from `IPC_PROTOCOL_VERSION` when it cannot be determined, to avoid spurious version mismatches between identical builds without git metadata.
## Individual Comments
### Comment 1
<location path="src/ipc.rs" line_range="88-97" />
<code_context>
}
+ // First try to decode just the version string to detect mismatches gracefully
+ match bincode::decode_from_slice::<String, _>(&buffer, bincode::config::standard()) {
+ Ok((version, _)) => {
+ if version != crate::ipc_types::IPC_PROTOCOL_VERSION {
+ let _ = send_response(
+ &mut stream,
+ &IpcResponse::err(format!(
+ "version mismatch: client is {}, server is {}. Please ensure instantwmctl and instantWM are the same version.",
+ version, crate::ipc_types::IPC_PROTOCOL_VERSION
+ )),
+ );
+ return;
+ }
+ }
+ Err(_) => {
+ // If we can't even decode a string, it's definitely a mismatch or garbage
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** The initial version-string decode error is silently ignored and the code continues as if nothing happened.
On `Err(_)` we fall through and still try to decode the full `IpcRequest`, which contradicts the comment and may surface a second, less-specific error to the client. Either return early on `Err(_)` with a clear mismatch/garbage error, or adjust the comment and logic to clearly express an intentional best-effort fallback (e.g. for backward compatibility).
</issue_to_address>
### Comment 2
<location path="src/bin/instantwmctl.rs" line_range="754-755" />
<code_context>
Ok((r, _)) => r,
Err(e) => {
eprintln!("instantwmctl: deserialization failed: {}", e);
+ eprintln!("This is likely caused by a version mismatch between instantwmctl and instantWM.");
+ eprintln!("Please ensure they are built from the same commit.");
std::process::exit(1);
}
</code_context>
<issue_to_address>
**suggestion:** The version-mismatch hint could be more actionable by including the detected protocol or build versions.
Since `IPC_PROTOCOL_VERSION` and `INSTANTWM_BUILD_COMMIT` are already embedded via build.rs, consider printing those (and/or the client’s equivalents) with this hint so users can immediately see which versions/commits are mismatched without extra investigation.
Suggested implementation:
```rust
Ok((r, _)) => r,
Err(e) => {
eprintln!("instantwmctl: deserialization failed: {}", e);
eprintln!("This is likely caused by a version mismatch between instantwmctl and instantWM.");
eprintln!(
"instantwmctl: IPC protocol version: {}, build commit: {}",
IPC_PROTOCOL_VERSION,
INSTANTWM_BUILD_COMMIT
);
eprintln!("Please ensure instantwmctl and instantWM are built from the same commit and use the same IPC protocol version.");
std::process::exit(1);
}
};
```
1. Ensure `IPC_PROTOCOL_VERSION` and `INSTANTWM_BUILD_COMMIT` are in scope in `src/bin/instantwmctl.rs`. If they are defined in the crate root or another module, you may need to add:
- `use crate::IPC_PROTOCOL_VERSION;`
- `use crate::INSTANTWM_BUILD_COMMIT;`
or equivalent module paths at the top of this file.
2. If the constants are exposed under a different namespace (e.g. `instantwm::IPC_PROTOCOL_VERSION`), adjust the references in the new `eprintln!` call accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+88
to
+97
| match bincode::decode_from_slice::<String, _>(&buffer, bincode::config::standard()) { | ||
| Ok((version, _)) => { | ||
| if version != crate::ipc_types::IPC_PROTOCOL_VERSION { | ||
| let _ = send_response( | ||
| &mut stream, | ||
| &IpcResponse::err(format!( | ||
| "version mismatch: client is {}, server is {}. Please ensure instantwmctl and instantWM are the same version.", | ||
| version, crate::ipc_types::IPC_PROTOCOL_VERSION | ||
| )), | ||
| ); |
There was a problem hiding this comment.
issue (bug_risk): The initial version-string decode error is silently ignored and the code continues as if nothing happened.
On Err(_) we fall through and still try to decode the full IpcRequest, which contradicts the comment and may surface a second, less-specific error to the client. Either return early on Err(_) with a clear mismatch/garbage error, or adjust the comment and logic to clearly express an intentional best-effort fallback (e.g. for backward compatibility).
Comment on lines
+754
to
+755
| eprintln!("This is likely caused by a version mismatch between instantwmctl and instantWM."); | ||
| eprintln!("Please ensure they are built from the same commit."); |
There was a problem hiding this comment.
suggestion: The version-mismatch hint could be more actionable by including the detected protocol or build versions.
Since IPC_PROTOCOL_VERSION and INSTANTWM_BUILD_COMMIT are already embedded via build.rs, consider printing those (and/or the client’s equivalents) with this hint so users can immediately see which versions/commits are mismatched without extra investigation.
Suggested implementation:
Ok((r, _)) => r,
Err(e) => {
eprintln!("instantwmctl: deserialization failed: {}", e);
eprintln!("This is likely caused by a version mismatch between instantwmctl and instantWM.");
eprintln!(
"instantwmctl: IPC protocol version: {}, build commit: {}",
IPC_PROTOCOL_VERSION,
INSTANTWM_BUILD_COMMIT
);
eprintln!("Please ensure instantwmctl and instantWM are built from the same commit and use the same IPC protocol version.");
std::process::exit(1);
}
};- Ensure
IPC_PROTOCOL_VERSIONandINSTANTWM_BUILD_COMMITare in scope insrc/bin/instantwmctl.rs. If they are defined in the crate root or another module, you may need to add:use crate::IPC_PROTOCOL_VERSION;use crate::INSTANTWM_BUILD_COMMIT;
or equivalent module paths at the top of this file.
- If the constants are exposed under a different namespace (e.g.
instantwm::IPC_PROTOCOL_VERSION), adjust the references in the neweprintln!call accordingly.
Contributor
I've resolved the merge conflict by applying the bincode deserialization fix directly to |
When the `instantwmctl` client and the `instantwm` server were built from different commits (resulting in different struct/enum layouts), `bincode` would fail during deserialization with a generic `UnexpectedVariant` error. This commit implements two fixes: 1. **Protocol Versioning**: Appends the first 8 characters of the current git commit hash to the `IPC_PROTOCOL_VERSION` generated in `build.rs`. This ensures different builds uniquely identify themselves. 2. **Graceful Deserialization**: In the server IPC handler (`src/ipc/mod.rs`), we now partially decode just the version `String` from the byte buffer *before* attempting to deserialize the entire `IpcRequest` struct. If a mismatch is detected, we can immediately return a friendly error message instead of letting bincode crash. 3. **Friendly Errors**: The client-side (`instantwmctl.rs`) now provides a clearer error message indicating a version mismatch when deserialization fails. Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
Member
Author
|
@jules |
When the `instantwmctl` client and the `instantwm` server were built from different commits (resulting in different struct/enum layouts), `bincode` would fail during deserialization with a generic `UnexpectedVariant` error. This commit implements two fixes: 1. **Protocol Versioning**: Appends the first 8 characters of the current git commit hash to the `IPC_PROTOCOL_VERSION` generated in `build.rs`. This ensures different builds uniquely identify themselves. 2. **Graceful Deserialization**: In the server IPC handler (`src/ipc/mod.rs`), we now partially decode just the version `String` from the byte buffer *before* attempting to deserialize the entire `IpcRequest` struct. If a mismatch is detected, we can immediately return a friendly error message instead of letting bincode crash. 3. **Friendly Errors**: The client-side (`instantwmctl.rs`) now provides a clearer error message indicating a version mismatch when deserialization fails. Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
Member
Author
|
gggg |
Contributor
Acknowledged. |
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.
This PR fixes an issue where the
instantwmctltool would throw a confusingUnexpectedVarianterror when communicating with an older/newerinstantwminstance due to a bincode struct layout mismatch.Changes:
build.rs: Modified the IPC protocol version generation to include the git commit hash. This makes versioning more robust between builds.src/ipc.rs: The IPC server now safely decodes only the version string from the buffer first. If it detects a version mismatch, it bails out early with a clear error message instead of trying to parse the mismatched struct variants and failing cryptically.src/bin/instantwmctl.rs: Updated the generic deserialize error message to provide a helpful hint that it is likely a version mismatch.PR created automatically by Jules for task 8945557543414012940 started by @paperbenni
Summary by Sourcery
Improve IPC protocol compatibility handling between instantwmctl clients and instantWM servers to surface clear version mismatch errors instead of low-level deserialization failures.
Bug Fixes:
Enhancements:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor