Repository navigation
Retry interrupted SSH uploads using a fresh connection - #880
Open
Masterwow3 wants to merge 1 commit into
Open
Masterwow3 wants to merge 1 commit into
Masterwow3 wants to merge 1 commit into
Conversation
m90
reviewed
Oct 9, 2026
m90
left a comment
m90
left a comment
Member
There was a problem hiding this comment.
Thanks for this. I think this is a nice feature to have, but considering stability, I'd prefer starting with a default value of 0 and letting users opt into this behavior. See the comment inline. Also, there is a linting error in the test.
Let me know if there are any questions.
| SSHIdentityFile string `split_words:"true" default:"/root/.ssh/id_rsa"` | ||
| SSHIdentityPassphrase string `split_words:"true"` | ||
| SSHRemotePath string `split_words:"true"` | ||
| SSHUploadRetries WholeNumber `split_words:"true" default:"2"` |
Member
There was a problem hiding this comment.
Considering the current behavior is not retrying, I would feel more comfortable with setting a default of 0 and allow users that need/want this to opt in by setting a value themselves.
| // NewStorageBackend creates and initializes a new SSH storage backend. | ||
| func NewStorageBackend(opts Config, logFunc storage.Log) (storage.Backend, func() error, error) { | ||
| if opts.UploadRetries < 0 || opts.UploadRetryDelay < 0 { | ||
| return nil, noop, errwrap.Wrap(nil, "SSH upload retries and retry delay must not be negative") |
Member
There was a problem hiding this comment.
I don't think this case is really needed as the WholeNumber type used in the config will bail on anything negative.
This branch has not been deployed
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
2 participants
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.
An SSH/SFTP connection loss during upload currently fails the entire backup run, even when the archive has already been created and the containers have restarted. The keepalive added in #863 helps prevent idle disconnects but does not reconnect after a transport failure.
This change retries the existing archive over a fresh SSH/SFTP connection. Each attempt starts at byte zero and truncates the destination file. It does not recreate the archive or run container lifecycle hooks again.
SSH_UPLOAD_RETRIES(default:2;0disables retries) andSSH_UPLOAD_RETRY_DELAY(default:5s).Validation:
go test ./...go vet ./...GOOS=linux GOARCH=amd64 go build ./...The Docker Compose integration suite was not run because no Docker daemon was available in the development environment. The SSH/SFTP regression test runs as part of the Go test suite without Docker.
Scope: this change does not retry initial backend setup or pruning, retain temporary archives after retries are exhausted, or publish remote files by atomic rename. An exhausted upload can still leave a partial remote file, as before. Existing SSH connection/handshake timeout behavior is unchanged.