diff --git a/internal/testutil/dockertestutil.go b/internal/testutil/dockertestutil.go index 8b22acda6..6b69d8ae7 100644 --- a/internal/testutil/dockertestutil.go +++ b/internal/testutil/dockertestutil.go @@ -50,7 +50,7 @@ func RunContainerAndKillOnCloseOrSkip(t *testing.T, args ...string) string { t.Cleanup(func() { // t.Context() is canceled by the time cleanup executes, so it cannot be used here - runDockerAndGetOutputOrSkip(context.Background(), t, "kill", containerID) + runDockerAndGetOutputOrSkip(context.WithoutCancel(t.Context()), t, "kill", containerID) }) return containerID diff --git a/repo/blob/rclone/rclone_storage.go b/repo/blob/rclone/rclone_storage.go index 77d2a4358..b59bed2b6 100644 --- a/repo/blob/rclone/rclone_storage.go +++ b/repo/blob/rclone/rclone_storage.go @@ -339,7 +339,7 @@ func New(ctx context.Context, opt *Options, isCreate bool) (blob.Storage, error) "--vfs-write-back=0s", // disable write-back, critical for correctness ) - r.cmd = exec.CommandContext(ctx, rcloneExe, arguments...) //nolint:gosec + r.cmd = exec.CommandContext(context.WithoutCancel(ctx), rcloneExe, arguments...) //nolint:gosec r.cmd.Env = append(r.cmd.Env, opt.RCloneEnv...) // https://github.com/kopia/kopia/issues/1934 diff --git a/repo/blob/rclone/rclone_storage_test.go b/repo/blob/rclone/rclone_storage_test.go index 7bb6ae79c..50b3bc81d 100644 --- a/repo/blob/rclone/rclone_storage_test.go +++ b/repo/blob/rclone/rclone_storage_test.go @@ -62,6 +62,38 @@ func mustGetRcloneExeOrSkip(t *testing.T) string { return rcloneExe } +func TestRCloneStorageCancelContext(t *testing.T) { + t.Parallel() + testutil.ProviderTest(t) + + rcloneExe := mustGetRcloneExeOrSkip(t) + dataDir := testutil.TempDirectory(t) + ctx := testlogging.Context(t) + + // use context that gets canceled after opening storage to ensure it's not used beyond New(). + newCtx, cancel := context.WithCancel(ctx) + st, err := rclone.New(newCtx, &rclone.Options{ + // pass local file as remote path. + RemotePath: dataDir, + RCloneExe: rcloneExe, + }, true) + + cancel() + + require.NoError(t, err, "unable to connect to rclone backend") + require.NotNil(t, st, "unable to connect to rclone backend") + + t.Cleanup(func() { + st.Close(testlogging.ContextForCleanup(t)) + }) + + var tmp gather.WriteBuffer + defer tmp.Close() + + err = st.GetBlob(ctx, blob.ID(uuid.New().String()), 0, -1, &tmp) + require.ErrorIs(t, err, blob.ErrBlobNotFound, "unexpected error when downloading non-existent blob") +} + func TestRCloneStorage(t *testing.T) { t.Parallel() testutil.ProviderTest(t) @@ -72,8 +104,8 @@ func TestRCloneStorage(t *testing.T) { dataDir := testutil.TempDirectory(t) // use context that gets canceled after opening storage to ensure it's not used beyond New(). - newctx, cancel := context.WithCancel(ctx) - st, err := rclone.New(newctx, &rclone.Options{ + newCtx, cancel := context.WithCancel(ctx) + st, err := rclone.New(newCtx, &rclone.Options{ // pass local file as remote path. RemotePath: dataDir, RCloneExe: rcloneExe, diff --git a/repo/blob/sftp/sftp_storage.go b/repo/blob/sftp/sftp_storage.go index b1ebfd54f..a4d2cb527 100644 --- a/repo/blob/sftp/sftp_storage.go +++ b/repo/blob/sftp/sftp_storage.go @@ -553,7 +553,9 @@ func New(ctx context.Context, opts *Options, isCreate bool) (blob.Storage, error impl.rec = connection.NewReconnector(impl) - conn, err := impl.rec.GetOrOpenConnection(ctx) + // removing cancelation from ctx since ctx is likely to be canceled after + // New returns, causing the initial connection to be closed and not reused + conn, err := impl.rec.GetOrOpenConnection(context.WithoutCancel(ctx)) if err != nil { return nil, errors.Wrap(err, "unable to open SFTP storage") } diff --git a/repo/blob/sftp/sftp_storage_test.go b/repo/blob/sftp/sftp_storage_test.go index 002a13aaa..306473ca2 100644 --- a/repo/blob/sftp/sftp_storage_test.go +++ b/repo/blob/sftp/sftp_storage_test.go @@ -212,8 +212,9 @@ func TestSFTPStorageValid(t *testing.T) { t.Run("PasswordCreds", func(t *testing.T) { ctx := testlogging.Context(t) + newctx, cancel := context.WithCancel(ctx) - st, err := createSFTPStorage(ctx, t, sftp.Options{ + st, err := createSFTPStorage(newctx, t, sftp.Options{ Path: "/upload2", Host: host, Username: sftpUsernameWithPasswordAuth, @@ -226,6 +227,8 @@ func TestSFTPStorageValid(t *testing.T) { t.Fatalf("unable to connect to SSH: %v", err) } + cancel() + deleteBlobs(ctx, t, st) blobtesting.VerifyStorage(ctx, t, st, blob.PutOptions{})