refactor(ssh): options to struct and handle ssh exit codes #447
Conversation
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
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
📝 WalkthroughWalkthroughThis PR refactors the SSH command execution API by replacing positional parameters with a structured Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Signed-off-by: Samuel K <skevetter@pm.me>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/ssh/helper.go`:
- Around line 165-179: setupContextCancellation currently spawns a goroutine
that blocks on ctx.Done() and can leak if the SSH command finishes while ctx
remains alive; change setupContextCancellation to return a cleanup function
along with error (e.g., func setupContextCancellation(ctx context.Context, sess
*ssh.Session) (func(), error)), create a done channel (or rename exit) that the
goroutine selects on alongside ctx.Done(), and have the returned cleanup close
that channel to ensure the goroutine exits; keep the behavior of calling
sess.Signal(ssh.SIGINT) when ctx.Done() fires, and update callers to defer the
returned cleanup after Run/command completion.
- Around line 181-209: handleRunError currently suppresses signal-like exit
codes (129/130/143) unconditionally; change it to only treat those as non-errors
when the operation was actually canceled by context. Modify handleRunError to
accept a context.Context (e.g., handleRunError(ctx context.Context, err error,
command string)), and when errors.As finds an *ssh.ExitError and the exitCode is
129/130/143, check ctx.Err() (or ctx.Err() == context.Canceled) before returning
nil; otherwise return the existing &ExitError{ExitCode: exitCode, Err: exitErr}.
Keep all other error-wrapping behavior intact.
Comment on lines
+165
to
+179
| func setupContextCancellation(ctx context.Context, sess *ssh.Session) error { | ||
| if err := ctx.Err(); err != nil { | ||
| return fmt.Errorf("context already cancelled: %w", err) | ||
| } | ||
| exit := make(chan struct{}) | ||
| defer close(exit) | ||
| go func() { | ||
| defer close(exit) | ||
| select { | ||
| case <-ctx.Done(): | ||
| _ = sess.Signal(ssh.SIGINT) | ||
| _ = sess.Close() | ||
| _ = sess.Signal(ssh.SIGINT) // Send interrupt, let defer handle close | ||
| case <-exit: | ||
| } | ||
| }() | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Avoid goroutine leak when context isn’t canceled.
The goroutine waits on ctx.Done() forever if the command completes while the context stays alive, which can leak goroutines on long‑lived contexts. Consider returning a cleanup function and closing it after Run completes.
🛠️ Proposed fix
- if err := setupContextCancellation(opts.Context, sess); err != nil {
- return err
- }
+ stopCancel, err := setupContextCancellation(opts.Context, sess)
+ if err != nil {
+ return err
+ }
+ defer stopCancel()
...
-func setupContextCancellation(ctx context.Context, sess *ssh.Session) error {
+func setupContextCancellation(ctx context.Context, sess *ssh.Session) (func(), error) {
if err := ctx.Err(); err != nil {
- return fmt.Errorf("context already cancelled: %w", err)
+ return func() {}, fmt.Errorf("context already cancelled: %w", err)
}
exit := make(chan struct{})
go func() {
defer close(exit)
select {
case <-ctx.Done():
_ = sess.Signal(ssh.SIGINT) // Send interrupt, let defer handle close
case <-exit:
}
}()
- return nil
+ return func() { close(exit) }, nil
}🤖 Prompt for AI Agents
In `@pkg/ssh/helper.go` around lines 165 - 179, setupContextCancellation currently
spawns a goroutine that blocks on ctx.Done() and can leak if the SSH command
finishes while ctx remains alive; change setupContextCancellation to return a
cleanup function along with error (e.g., func setupContextCancellation(ctx
context.Context, sess *ssh.Session) (func(), error)), create a done channel (or
rename exit) that the goroutine selects on alongside ctx.Done(), and have the
returned cleanup close that channel to ensure the goroutine exits; keep the
behavior of calling sess.Signal(ssh.SIGINT) when ctx.Done() fires, and update
callers to defer the returned cleanup after Run/command completion.
Comment on lines
+181
to
209
| func handleRunError(err error, command string) error { | ||
| // Check for exit errors with exit codes | ||
| var exitErr *ssh.ExitError | ||
| if errors.As(err, &exitErr) { | ||
| exitCode := exitErr.ExitStatus() | ||
|
|
||
| // Exit codes 128+N indicate death by signal N | ||
| // 130 = 128 + 2 (SIGINT) - Ctrl+C (user interrupted) | ||
| // 129 = 128 + 1 (SIGHUP) - hangup (terminal closed) | ||
| // 143 = 128 + 15 (SIGTERM) - graceful termination | ||
| // These are "normal" ways to exit an interactive session | ||
| if exitCode == 130 || exitCode == 129 || exitCode == 143 { | ||
| return nil // Don't treat user interrupts as errors | ||
| } | ||
|
|
||
| // Return exit code for all other cases | ||
| return &ExitError{ | ||
| ExitCode: exitCode, | ||
| Err: exitErr, | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| // Provide context for common errors | ||
| if errors.Is(err, io.EOF) { | ||
| return fmt.Errorf("SSH session closed unexpectedly (EOF) while running: %s", command) | ||
| } | ||
|
|
||
| return fmt.Errorf("SSH command failed: %w (command: %s)", err, command) | ||
| } |
There was a problem hiding this comment.
Don’t swallow signal exit codes unless cancellation was requested.
Returning nil for 129/130/143 unconditionally can mask genuine failures (e.g., remote SIGTERM). Consider only suppressing these when the context is actually canceled, otherwise propagate ExitError.
🛠️ Proposed fix
- return handleRunError(err, opts.Command)
+ return handleRunError(opts.Context, err, opts.Command)
...
-func handleRunError(err error, command string) error {
+func handleRunError(ctx context.Context, err error, command string) error {
// Check for exit errors with exit codes
var exitErr *ssh.ExitError
if errors.As(err, &exitErr) {
exitCode := exitErr.ExitStatus()
@@
- if exitCode == 130 || exitCode == 129 || exitCode == 143 {
- return nil // Don't treat user interrupts as errors
- }
+ if exitCode == 130 || exitCode == 129 || exitCode == 143 {
+ if ctx.Err() != nil {
+ return nil // Don't treat user interrupts as errors
+ }
+ return &ExitError{ExitCode: exitCode, Err: exitErr}
+ }🤖 Prompt for AI Agents
In `@pkg/ssh/helper.go` around lines 181 - 209, handleRunError currently
suppresses signal-like exit codes (129/130/143) unconditionally; change it to
only treat those as non-errors when the operation was actually canceled by
context. Modify handleRunError to accept a context.Context (e.g.,
handleRunError(ctx context.Context, err error, command string)), and when
errors.As finds an *ssh.ExitError and the exitCode is 129/130/143, check
ctx.Err() (or ctx.Err() == context.Canceled) before returning nil; otherwise
return the existing &ExitError{ExitCode: exitCode, Err: exitErr}. Keep all other
error-wrapping behavior intact.
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.
Summary by CodeRabbit