Skip to content

Pass maxdepth through in AsyncFileSystem._rm - #2106

Open
ckarnell wants to merge 1 commit into
fsspec:masterfrom
ckarnell:fix-async-rm-maxdepth
Open

Pass maxdepth through in AsyncFileSystem._rm#2106
ckarnell wants to merge 1 commit into
fsspec:masterfrom
ckarnell:fix-async-rm-maxdepth

Conversation

@ckarnell

Copy link
Copy Markdown

AsyncFileSystem._rm accepts maxdepth into **kwargs and never uses it, so async filesystems delete deeper than the caller asked for.

async def _rm(self, path, recursive=False, batch_size=None, **kwargs):
    batch_size = batch_size or self.batch_size
    path = await self._expand_path(path, recursive=recursive)

The sync twin passes it on:

def rm(self, path, recursive=False, maxdepth=None):
    path = self.expand_path(path, recursive=recursive, maxdepth=maxdepth)

maxdepth bounds which paths get collected for deletion, and its own docstring says that without it "there will be no limit and infinite recursion may be possible". On the async side it lands in **kwargs and is forwarded to _rm_file, which has no use for it, while _expand_path runs unbounded.

With a tree of /root/a.txt, /root/d1/b.txt and /root/d1/d2/c.txt, calling rm("/root", recursive=True, maxdepth=1) on the same primitives:

sync   deleted /root, /root/a.txt, /root/d1
async  deleted /root, /root/a.txt, /root/d1, /root/d1/b.txt, /root/d1/d2, /root/d1/d2/c.txt

Three files the bound was meant to protect. That's data loss. This reaches any backend implementing _rm_file and inheriting _rm, which is the usual pattern, and delete() is an alias of rm carrying maxdepth too.

Test is test_rm_honours_maxdepth, using the same shape as the existing _CatRangesFS case. Suite goes 1556 to 1557 on fsspec/tests and fsspec/implementations/tests. Reverting only asyn.py fails that one test, so it's the fix being measured. ruff check and ruff format are clean under the 0.14.3 you pin in pre-commit.

This only touches the base class. A backend overriding _rm with its own signature needs the same change, and I have not run this against a real cloud backend.

AsyncFileSystem._rm accepted maxdepth into **kwargs and never used it, so async
filesystems deleted deeper than the caller asked for. The sync twin passes it to
expand_path; this does the same. Adds test_rm_honours_maxdepth.

@fallenmi fallenmi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verified on exact head d8402ab64320792f22f4c3b1e0444674bb3d0fff against merge base c45f6011c37a74e4cf64d4378bad3e0af38382d0.

Using the same three-level AsyncFileSystem primitive as the regression, the base schedules all six paths for rm("/root", recursive=True, maxdepth=1), including /root/d1/b.txt and /root/d1/d2/c.txt. This head schedules only /root, /root/a.txt, and /root/d1; with maxdepth=None, it still schedules all six, so the unbounded path is unchanged. The new regression passes, and the full fsspec/tests/test_async.py target is 28 passed on Python 3.13.5.

The argument is now consumed by _expand_path instead of leaking through per-file kwargs, matching synchronous AbstractFileSystem.rm. Looks correct to me.

Reviewed with OpenAI Codex assistance; I independently inspected the source and ran the checks above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants