fix(pam): fix SCP returning exit code 1 despite successful transfer#170
fix(pam): fix SCP returning exit code 1 despite successful transfer#170devin-ai-integration[bot] wants to merge 2 commits intomainfrom
Conversation
Previously, handleChannel only waited for one direction of the data copy to finish before starting channel teardown. For SCP operations, the client→server copy finishes first (file data sent), but the server has not yet delivered exit-status. The 500ms timeout would fire, channels were closed, and exit-status was lost — causing SCP to report exit code 1 despite success. Changes: - Wait for both data copy directions to complete before teardown - Call CloseWrite on server channel after client→server copy finishes so the remote SCP process receives EOF and can exit cleanly - Increase the server-requests timeout from 500ms to 3s as a safety net Co-Authored-By: Andrey Lyubavin <andrey@infisical.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Greptile SummaryThis PR fixes a long-standing SCP exit-code-1 bug by making the channel teardown wait for both data-copy directions (instead of just one), calling Confidence Score: 5/5Safe to merge — the fix is logically sound and all remaining findings are minor P2 style suggestions. No P0 or P1 issues found. The two comments cover a silently-dropped CloseWrite error (easy to add logging) and a duplicate log line on context cancellation. Neither affects correctness or reliability of the fix. No files require special attention beyond the two P2 nits in packages/pam/handlers/ssh/proxy.go. Important Files Changed
Reviews (1): Last reviewed commit: "fix(pam): wait for both data directions ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
packages/pam/handlers/ssh/proxy.go
Outdated
| 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() |
There was a problem hiding this comment.
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.
| serverChannel.CloseWrite() | |
| if err := serverChannel.CloseWrite(); err != nil { | |
| log.Debug().Err(err).Str("sessionID", sessionID).Msg("Failed to CloseWrite on server channel") | |
| } |
There was a problem hiding this comment.
Good catch — added error logging for CloseWrite() in c619d69.
| 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") | ||
| } |
There was a problem hiding this comment.
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:
| 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 | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Good point — added a cancelled flag to deduplicate the log in c619d69.
…ancel log Co-Authored-By: Andrey Lyubavin <andrey@infisical.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Description 📣
Fixes an issue where SCP (and potentially SFTP/rsync) through the PAM SSH proxy returns exit code 1 even when the file transfer succeeds.
Root cause: In
handleChannel, the proxy only waited for one direction of the bidirectional data copy to finish before starting channel teardown. For SCP uploads, the client→server copy finishes first (all file data sent), but the remotescpprocess hasn't exited yet — it still needs to send theexit-statuschannel request. The old code then waited only 500ms forserverReqDonebefore closing both channels. If theexit-statushadn't been forwarded yet, the client never received it and defaulted to exit code 1.Fix (3 parts):
exit-status) is observed.serverChannel.CloseWrite()after the client→server copy finishes, signaling EOF to the remote process so it can exit and deliverexit-status.Updates since last revision:
CloseWrite()error is now logged at Debug level instead of being silently dropped.cancelledflag to avoid duplicate "Channel cancelled by context" log entries when ctx fires on both loop iterations.Type ✨
Tests 🛠️
go build ./...)infisical pam ssh proxyand perform SCP through the proxy; verify exit code is 0 on success.Human review checklist
CloseWrite()on the server channel have any adverse effect on interactive SSH sessions or SFTP subsystems? (It's called afterproxyDatareturns, meaning the client already sent EOF, so it should be safe — but worth verifying.)serverReqDone?Link to Devin session: https://app.devin.ai/sessions/e7ba74ee0e7546a6a6855a7b5eed6de1