Skip to content

check_file_permissions: probe inside target store, not its parent (#1098)#1676

Merged
LOCEANlloydizard merged 2 commits into
echostack-org:mainfrom
gaoflow:fix/1098-check-file-permissions-target-dir
Jun 9, 2026
Merged

check_file_permissions: probe inside target store, not its parent (#1098)#1676
LOCEANlloydizard merged 2 commits into
echostack-org:mainfrom
gaoflow:fix/1098-check-file-permissions-target-dir

Conversation

@gaoflow

@gaoflow gaoflow commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Closes #1098.

Problem

check_file_permissions writes its probe file to the parent of the target directory instead of the target itself when given an FSMap (the zarr/cloud path). As reported in #1098, for a target of .../combined_files the probe was created at .../ :

File Directory Root :  .../notebooks/combined_files
Test file generated at :  .../notebooks/.f0605b17-...

This means the permission check validates the wrong directory, and (when parent and target permissions differ) can pass or fail incorrectly.

Cause

The FSMap branch used base_dir = os.path.dirname(FILE_DIR.root), which strips the last path component, so the probe lands one level up. The Path/str branch right below it correctly writes inside the target (and mkdir(parents=True) first).

os.path.dirname was presumably used because the target store may not exist yet (e.g. a not-yet-written zarr store), and writing inside a missing directory raises — but that's exactly the case the Path/str branch already handles by creating the directory first.

Fix

Write the probe inside FILE_DIR.root and makedirs(..., exist_ok=True) first, mirroring the Path/str branch. This keeps the check on the actual target and still works when the store doesn't exist yet (it gets created, as the local-path branch already does).

Tests

Added two regression tests (red before, green after):

  • test_check_file_permissions_fsmap_writes_inside_target — the probe is opened inside the target store, not its parent.
  • test_check_file_permissions_fsmap_creates_missing_store — a not-yet-created store is created and passes the check.

Full test_utils_io.py passes (47 passed, 2 xfailed).

gaoflow added 2 commits June 7, 2026 13:05
The probe path uses fsspec's forward-slash convention, so on Windows the
parent-dir assertion compared '/'-separated paths against os.sep '\\'.
Normalize both sides before comparing.
@LOCEANlloydizard

Copy link
Copy Markdown
Collaborator

Hi @gaoflow, thanks! the fix and tests both look good to me! cheers!

@LOCEANlloydizard LOCEANlloydizard merged commit 50e7a6f into echostack-org:main Jun 9, 2026
12 checks passed
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.

Incorrect Path for Temporary File Generation in check_file_permissions Method

2 participants