[RUN-3112] Fix sudo hang and exit-code parse crash in SSHJ executor#111
Open
ncofreortiz-hub wants to merge 3 commits into
Open
[RUN-3112] Fix sudo hang and exit-code parse crash in SSHJ executor#111ncofreortiz-hub wants to merge 3 commits into
ncofreortiz-hub wants to merge 3 commits into
Conversation
Three bugs in the sudo flow added in 5.8.0 (PR #66): 1. Expect timeout was set to 30000 TimeUnit.SECONDS (~8.3h) instead of milliseconds, so a failed prompt match looked like an infinite hang. 2. PROMPT_PATTERN "~.*\$" assumed every shell prompt contains a literal '~' and ends in '$', which fails for root shells (#) or custom PS1, causing the initial expect() to never match. 3. Leaked ANSI control-sequence remnants (e.g. bracketed-paste-mode toggles) in the captured exit-code response crashed with NumberFormatException: For input string: "[?2004l0" since the response was fed into Integer.parseInt() unsanitized. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses customer-reported hangs and exit-code parsing failures in the SSHJ sudo-over-PTY flow by tightening Expect timeout configuration, broadening prompt detection, and sanitizing exit-code output to avoid ANSI/control-sequence contamination.
Changes:
- Fixes the Expect timeout unit to milliseconds so prompt-matching failures fail fast.
- Updates prompt matching and adds exit-code sanitization/validation to prevent parse crashes.
- Adds Spock coverage for prompt-regex and exit-code sanitization/validation behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/main/java/com/plugin/sshjplugin/sudo/SudoCommand.java | Adjusts prompt regex/timeout and adds exit-code sanitization + validation logic in the sudo shell flow. |
| src/test/groovy/com/plugin/sshjplugin/sudo/SudoCommandSpec.groovy | Adds tests for the new prompt regex and exit-code sanitization/validation helpers. |
…overflow - PROMPT_PATTERN lacked an end-of-input anchor, so expectit's find()-based matching could fire on any mid-output '$'/'#' (e.g. "PATH=$PATH:...") instead of the actual shell prompt. Anchored with $. - isValidExitCode only checked digit shape via regex, which still let an overflowing digit string through to the caller's Integer.parseInt(), reintroducing the crash this PR fixes. Now validates via parseInt itself. Co-Authored-By: Claude <noreply@anthropic.com>
fdevans
changed the title
ronaveva
reviewed
Jul 10, 2026
| // Was "30000, TimeUnit.SECONDS" (~8.3 hours) - a unit typo that made | ||
| // the sudo flow appear to hang indefinitely instead of failing fast | ||
| // when a prompt/pattern never matched. | ||
| .withTimeout(30000, TimeUnit.MILLISECONDS) |
Contributor
There was a problem hiding this comment.
Is this TO enough for any kind of command to be triggered? @ncofreortiz-hub
…timeout Reducing the single global expect timeout from 30000s to 30000ms fixed the hang on a broken prompt match, but it also capped every step at 30s - including the wait for the sudo'd command itself to finish, which the original (buggy) huge value was there to accommodate for long-running commands. Split it: prompt/password exchanges keep a short 30s fail-fast timeout, while the wait for the command to complete now uses the same user-configurable ssh-command-timeout (0 = unlimited) the non-sudo path already honors, via a new SudoCommand#setCommandTimeoutMs plumbed through SudoCommandBuilder from SSHJExec. Co-Authored-By: Claude <noreply@anthropic.com>
ronaveva
reviewed
Jul 10, 2026
| // prompt-detection default above. | ||
| Expect commandWait = commandTimeoutMs > 0 | ||
| ? expect.withTimeout(commandTimeoutMs, TimeUnit.MILLISECONDS) | ||
| : expect.withInfiniteTimeout(); |
Contributor
There was a problem hiding this comment.
Suggested change
| : expect.withInfiniteTimeout(); | |
| : expect.withTimeout(Long.MAX_VALUE, TimeUnit.DAYS); |
since withInfiniteTimeout is deprecated, we could use something like this
ronaveva
approved these changes
Jul 10, 2026
ronaveva
left a comment
ronaveva
left a comment
Contributor
There was a problem hiding this comment.
LGTM, make sure to solve the pending observation before merging
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
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.
Summary
Fixes the sudo-over-PTY flow introduced in PR #66 (Rundeck 5.8.0, RPL-40), which customers reported as either hanging forever or failing with
Failed: Unknown: For input string: "[?2004l0".Three separate defects in
SudoCommand.java, all still present onmain:.withTimeout(30000, TimeUnit.SECONDS)is ~8.3 hours, not 30 seconds. Any failed prompt match therefore looked like an infinite hang instead of failing fast. Fixed toTimeUnit.MILLISECONDS.PROMPT_PATTERN = "~.*\\$"assumes every shell prompt contains a literal~and ends in$. Root shells (#) and customPS1values never match, so the initialexpect()blocks. Broadened to.*[#$]\s*.expectit'sremoveNonPrintable()filter strips only the leading ESC byte of an ANSI control sequence (e.g. a bracketed-paste-mode toggle), leaving the visible remainder ([?2004l) glued to the real output. That string was fed directly intoInteger.parseInt()with no validation, producing the reportedNumberFormatException. AddedsanitizeExitCode()/isValidExitCode()to strip escape remnants and raise a clearIOExceptioninstead of an opaque parse crash if the response is still not a plain integer.Test plan
./gradlew compileJava compileTestGroovy— compiles clean./gradlew test --tests SudoCommandSpec --tests SSHJExecSpec— 14/14 pass (newSudoCommandSpeccovers all three fixes + regex/sanitize edge cases; existingSSHJExecSpecregression still green)🤖 Generated with Claude Code