rm does not delete the directory if its input is a directory with recursive True

[LuchiLucs]

As stated in the title, it seems to me, that the API rm does not remove the directory itself if the input path its a directory and the recursive flag is set to True:

  await self.fs._rm(
      path=f"{self.bucket_name}/{remote_path}",
      recursive=True,
      **kwargs,
  )

I checked by s3 bucket from the dashboard and the directory does still exists, while its content does not. The documentation says that:

recursive (bool) – If file(s) are directories, recursively delete contents and then also remove the directory

I guess the implementation does not follow the documentation and a direct rmdir API call is needed in order to delete also the directory.

[LuchiLucs]

In the debugger I saw that:

    async def _rm(self, path, recursive=False, **kwargs):
        if recursive and isinstance(path, str):
            bucket, key, _ = self.split_path(path)
            if not key and await self._is_bucket_versioned(bucket):
                # special path to completely remove versioned bucket
                await self._rm_versioned_bucket_contents(bucket)
        paths = await self._expand_path(path, recursive=recursive)
        files = [p for p in paths if self.split_path(p)[1]]
        dirs = [p for p in paths if not self.split_path(p)[1]]

In this code, all the paths (real dirs and files) are detected in the files variable hwile the dirs variable is an empty list

[martindurant]

It seems expand_path is not recovering the zero-length directory placeholder with "/" suffix. We could arbitarily add it here, since attempting to remove it if it doesn't exist shouldn't have an cost, or fix expand_path. I think I prefer the former: "path/" isn't really a member of "path".

[LuchiLucs]

If I try a direct call to the empy dir:

await self.fs._rmdir(
   path=f"{self.bucket_name}/{remote_path}",
)

There is the exception "FileExistsError". The path is empty though.

[martindurant]

It is

await self.fs._rm_file(
   path=f"{self.bucket_name}/{remote_path}/",
)

(with the "/" suffix) that you want. This is a FILE, which is exactly the problem.

[LuchiLucs]

It is

await self.fs._rm_file(
   path=f"{self.bucket_name}/{remote_path}/",
)

(with the "/" suffix) that you want. This is a FILE, which is exactly the problem.

I tried without success. If I use as value for the path either a "file path" as .../remote_path or a "dir path" as .../remote_path/ using either the _rm or the _rm_file API, the folder is not deleted, only its content is.
Is there a workaround to force the deletion of the folder itself too? Should I use aiobotocore directly?

Thanks

[ywilke]

I tried without success. If I use as value for the path either a "file path" as .../remote_path or a "dir path" as .../remote_path/ using either the _rm or the _rm_file API, the folder is not deleted, only its content is.
Is there a workaround to force the deletion of the folder itself too? Should I use aiobotocore directly?

@martindurant it seems like the real issue/bug is that rm_file() doesn't remove an empty prefix whether you include the trailing / or not.

It would make sense if self.fs.rm(path, recursive=True) would delete everything under that prefix and also the prefix itself.
Would love to see a fix for this!

[martindurant]

It would make sense if self.fs.rm(path, recursive=True) would delete everything under that prefix and also the prefix itself.
Would love to see a fix for this!

but you don't mean "prefix", but rather "file with the same name", no?

[ywilke]

Maybe I got the terms wrong but when you call:
self.fs.rm("s3://my-bucket/logs", recursive=True)
I would expect:
self.fs.exists("s3://my-bucket/logs")
to return False

Do you consider it a bug that empty "directories" are not removed by by rm(recursive=True) currently?

[martindurant]

fs.isdir(path) is False, right?

You may still have a point, but it is not a completely clear situation.

[ywilke]

No fs.isdir(path) returns True after rm().

[martindurant]

OK, that should not be so. A concrete reproducer in the form of a test we can add to the codebase would be very useful. Ideally, we should add rm() to the set of abstract bulk op tests in https://github.com/fsspec/filesystem_spec/tree/master/fsspec/tests/abstract to ensure consistent behaviour across backends.

[ywilke]

I have a small reproducible example here:

import s3fs
import boto3

bucket = "my-bucket" # replace with your bucket name
prefix = "logs/" # must end with "/"

# Create empty "directory" in S3
s3 = boto3.client("s3")
s3.put_object(
    Bucket=bucket,
    Key=prefix,
    Body=b""
)

# List path
fs = s3fs.S3FileSystem()
logs_path = f"s3://{bucket}/{prefix}"
print("ls before", fs.ls(logs_path))

# Try to remove
fs.rm(logs_path, recursive=True)

# Check if it exists
print("exists", fs.exists(logs_path))
print("isdir", fs.isdir(logs_path))
print("ls after", fs.ls(logs_path))

returns:

ls before ['my-bucket/logs/']
exists True
isdir True
ls after ['my-bucket/logs/']

If the prefix does not end in / then it is correctly deleted. So something is going wrong when it ends with /.

[martindurant]

I created #1004 with your failing test, and welcome suggestions on a fix.