fix(client): CodeRabbit follow-ups for the tenant port — update/uninstall race + exe-path slice#1360
Open
mikhailm-coder wants to merge 10 commits into
Open
fix(client): CodeRabbit follow-ups for the tenant port — update/uninstall race + exe-path slice#1360mikhailm-coder wants to merge 10 commits into
mikhailm-coder wants to merge 10 commits into
Conversation
…msh target) (#1981) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…(#2030) Co-authored-by: Danylo Babenko <danylobabenko@Danylos-MacBook-Pro.local>
- drop dead trailing assignment to reinstall_dir_cleared (unused_assignments; the flag is never read after the no-registry-record repair block) - mesh self-heal: map_or(false, ..) -> is_some_and(..) (clippy::unnecessary_map_or) Both warnings originate in code ported verbatim from openframe-oss-tenant; this keeps the repo clippy gate (-D warnings) green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011z7MGH2qYeKt2ZQV88EaD8
1. tool_agent_update_listener: serialize updates with the per-tool lock. The per-tool lock introduced with remote uninstall was held by install (blocking) and uninstall (try_lock -> defer via redelivery), but the update path never acquired it, so an uninstall arriving mid-update could remove the tool while binaries/state were being written. The listener now try_locks before dispatching (same pattern as the uninstall listener) and leaves the message unacked for JetStream redelivery when the tool is busy. 2. system_service::parse_exe_from_image_path: use to_ascii_lowercase(). to_lowercase() can change byte lengths for non-ASCII characters, so slicing the original string with an index found in the lowered string could return a wrong path or panic on a char boundary. Both issues exist identically in openframe-oss-tenant and should be fixed there too (or picked up when tenant consumes this lib). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011z7MGH2qYeKt2ZQV88EaD8
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
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
3 participants
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
Fixes both CodeRabbit findings from #1359 in one PR, as agreed there (kept out of #1359 so the port stays a byte-faithful mirror of tenant).
Stacked on
feat/port-tenant-client-updates(#1359); GitHub will retarget tomainwhen that merges.Fixes
tool_agent_update_listener). Previously install held the lock (blocking) and uninstall deferred on it (try_lock→ leave unacked → JetStream redelivery), but updates never acquired it — an uninstall arriving mid-update could delete the tool while binaries/state were being written. The update listener now mirrors the uninstall listener exactly:try_lockbefore dispatching, defer via redelivery when busy. No blocking in the listener loop, so updates for other tools aren't head-of-line blocked.parse_exe_from_image_path:to_lowercase()→to_ascii_lowercase()— the lowered string's byte index was used to slice the original string; non-ASCII Windows paths could yield a wrong path or panic on a char boundary. ASCII lowering keeps indices aligned (.exematching needs only ASCII).Note for upstream
Both issues exist identically in
openframe-oss-tenanttoday; they should be fixed there too (or picked up when tenant consumes this lib).Gates
cargo clippy --all-targets --features bin -- -D warnings✅cargo fmt --all -- --check✅cargo test: 82 passed; the 2platform::directoriesfailures are pre-existing onmain(local-permissions-dependent), unrelated.🤖 Generated with Claude Code
https://claude.ai/code/session_011z7MGH2qYeKt2ZQV88EaD8