-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Add --fail-fast option to stop on first failure (pdsh -k compatible) #103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Security & Performance ReviewAnalysis Summary
Prioritized Fix RoadmapMEDIUM
LOW
Progress Log
Technical DetailsIssue: UTF-8 String Boundary Violation Current code: let short_error = if first_line.len() > 50 {
format!("{}...", &first_line[..47])
} else {
first_line.to_string()
};Problem: Proof of panic: let s = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\u{4e2d}\u{6587}"; // 45 ASCII + Chinese
s[..47] // PANICS: byte 47 is inside '中' (bytes 45..48)Fix approach: Use Reviewer: AI Code Reviewer |
Review CompleteProgress Log
Verification
SummaryThe PR implements a solid
One bug was found and fixed: potential panic on non-ASCII error messages when truncating for display. Overall Assessment: The implementation is well-designed and follows async Rust best practices. Ready for merge after the UTF-8 fix. |
8524cbd to
2962620
Compare
…ible) Add a new --fail-fast / -k option that stops execution immediately when any node fails (connection error or non-zero exit code). This provides pdsh compatibility and is useful for critical operations where partial execution is unacceptable. Implementation details: - Uses tokio::sync::watch channel for cancellation signaling - Uses tokio::select! to race between execution and cancellation - Cancels pending tasks waiting in semaphore queue - Reports which node caused the failure with error details - Works with existing parallel execution infrastructure Features: - Short option -k for pdsh compatibility - Long option --fail-fast for clarity - Can be combined with --require-all-success and --check-all-nodes - Supports all parallelism settings (including --parallel N) Closes #94
Priority: MEDIUM Issue: String slicing with byte index could panic on multi-byte UTF-8 chars The error message truncation in execute_with_fail_fast() used direct byte indexing (&first_line[..47]) which can panic if the index falls in the middle of a multi-byte UTF-8 character. Fixed by using floor_char_boundary(47) to find the largest valid char boundary at or before byte 47, ensuring safe string truncation for all Unicode content including CJK characters and emoji.
- Add --fail-fast / -k option description in manpage OPTIONS section - Add fail-fast examples in manpage EXAMPLES section - Add fail-fast mode implementation details in ARCHITECTURE.md
2962620 to
149666c
Compare
… cache The test helper was finding stale release binaries in CI, causing tests to fail because the old binary didn't have --connect-timeout option. Changed to prefer debug binary since `cargo test` builds debug binaries.
Summary
--fail-fast/-koption for pdsh compatibilityChanges
CLI (
src/cli.rs)--fail-fast/-kflag with help text explaining pdsh compatibilityExecutor (
src/executor/parallel.rs)fail_fastfield toParallelExecutorstructwith_fail_fast()builder methodexecute_with_fail_fast()method using:tokio::sync::watchchannel for cancellation signalingtokio::select!to race between semaphore acquisition and cancellationIntegration (
src/commands/exec.rs,src/app/dispatcher.rs)fail_fastparameter through the execution pipelineTests (
tests/fail_fast_test.rs)Documentation (
README.md)Test plan
cargo test fail_fast- All 10 tests passcargo clippy -- -D warnings- No warningscargo fmt- Code is properly formatted-kflag does not conflict with existing optionsRelated issues