Skip to content

fix(pam): fix SCP returning exit code 1 despite successful transfer #170

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.

Sign up for GitHub

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

Open
devin-ai-integration wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(pam): fix SCP returning exit code 1 despite successful transfer #170

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c7cfc6c
fix(pam): wait for both data directions before SSH channel teardown
devin-ai-integration[bot] Apr 2, 2026
c619d69
fix: address review comments - log CloseWrite error and deduplicate c…
devin-ai-integration[bot] Apr 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 21 additions & 9 deletions packages/pam/handlers/ssh/proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,9 @@ func (p *SSHProxy) handleChannel(ctx context.Context, newChannel ssh.NewChannel,
// Client to Server
go func() {
err := p.proxyData(clientChannel, serverChannel, "client→server", sessionID, true, chState)
// Signal the server that the client is done writing so the remote process
// receives EOF and can exit, which triggers exit-status delivery.
serverChannel.CloseWrite()
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 CloseWrite() error silently dropped

The error returned by serverChannel.CloseWrite() is discarded. If the channel is already half-closed or broken when this fires, the failure will be invisible in logs, making it harder to diagnose future issues.

Suggested change
serverChannel.CloseWrite()
if err := serverChannel.CloseWrite(); err != nil {
log.Debug().Err(err).Str("sessionID", sessionID).Msg("Failed to CloseWrite on server channel")
}

Copy link
Copy Markdown
Contributor Author

@devin-ai-integration devin-ai-integration Bot Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — added error logging for CloseWrite() in c619d69.

errChan <- err
}()

Expand All @@ -256,20 +259,29 @@ func (p *SSHProxy) handleChannel(ctx context.Context, newChannel ssh.NewChannel,
errChan <- err
}()

// Wait for either direction to finish or context cancellation
select {
case err := <-errChan:
if err != nil && err != io.EOF {
log.Debug().Err(err).Str("sessionID", sessionID).Msg("Channel proxy error")
// Wait for BOTH directions to finish (or context cancellation).
// Previously only one direction was awaited, which caused premature teardown
// for SCP: the client→server copy would finish first (file data sent), but the
// server had not yet delivered exit-status. Waiting for both directions ensures
// the server's data EOF (which follows exit-status) is observed before teardown.
for i := 0; i < 2; i++ {
select {
case err := <-errChan:
if err != nil && err != io.EOF {
log.Debug().Err(err).Str("sessionID", sessionID).Msg("Channel proxy error")
}
case <-ctx.Done():
log.Info().Str("sessionID", sessionID).Msg("Channel cancelled by context")
}
case <-ctx.Done():
log.Info().Str("sessionID", sessionID).Msg("Channel cancelled by context")
}
Comment on lines +270 to 282
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Duplicate log entry on context cancellation

If ctx is already cancelled when the loop starts (or is cancelled during the first iteration), ctx.Done() fires on both iterations and "Channel cancelled by context" is logged twice. Since errChan has capacity 2 the goroutines won't leak, but the duplicate Info-level log line can be confusing. Consider tracking whether cancellation was already observed:

Suggested change
for i := 0; i < 2; i++ {
select {
case err := <-errChan:
if err != nil && err != io.EOF {
log.Debug().Err(err).Str("sessionID", sessionID).Msg("Channel proxy error")
}
case <-ctx.Done():
log.Info().Str("sessionID", sessionID).Msg("Channel cancelled by context")
}
case <-ctx.Done():
log.Info().Str("sessionID", sessionID).Msg("Channel cancelled by context")
}
cancelled := false
for i := 0; i < 2; i++ {
select {
case err := <-errChan:
if err != nil && err != io.EOF {
log.Debug().Err(err).Str("sessionID", sessionID).Msg("Channel proxy error")
}
case <-ctx.Done():
if !cancelled {
log.Info().Str("sessionID", sessionID).Msg("Channel cancelled by context")
cancelled = true
}
}
}

Copy link
Copy Markdown
Contributor Author

@devin-ai-integration devin-ai-integration Bot Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point — added a cancelled flag to deduplicate the log in c619d69.


// Brief window for exit-status to be forwarded before channel teardown.
// Wait for the server-side channel requests handler to finish so that any
// remaining requests (exit-status, exit-signal) are forwarded to the client
// before we tear down the channels.
select {
case <-serverReqDone:
case <-time.After(500 * time.Millisecond):
case <-time.After(3 * time.Second):
log.Debug().Str("sessionID", sessionID).Msg("Timed out waiting for server requests to complete")
}
clientChannel.Close()
serverChannel.Close()
Expand Down
Loading