Skip to content

fix: preserve read-only hardlink attributes on Windows - #381

Open
karthiksenv wants to merge 3 commits into
treeverse:mainfrom
karthiksenv:codex/preserve-readonly-hardlinks
Open

karthiksenv wants to merge 3 commits into
treeverse:mainfrom
karthiksenv:codex/preserve-readonly-hardlinks

Conversation

@karthiksenv

@karthiksenv karthiksenv commented Nov 5, 2026 •

Copy link
Copy Markdown

Removing a read-only hardlink on Windows currently clears the read-only attribute before unlinking. That attribute is shared by the remaining links, so removing a workspace file can leave its cache object writable, as reported in treeverse/dvc#11119.

Use Windows extended file disposition for read-only regular-file unlink failures. This removes the requested name without clearing the shared attribute and also covers files removed through shutil.rmtree. The helper closes its handle on success or failure and preserves the original deletion error if closing also fails. Add direct/recursive hardlink regressions, failure-path tests, and a README compatibility note.

The handle-based approach follows the suggestion and independent reproduction in treeverse/dvc#11119 (comment).

Compatibility decision for review: if the OS or filesystem rejects this operation, removal raises an error rather than falling back to chmod. This applies to singly linked read-only regular files too. Such environments may previously have removed these files successfully; preserving shared attributes takes precedence in this change. Older Windows versions and network filesystems have not been exercised.

Validation on Windows, Python 3.12.15:

  • A disposable NTFS reproduction loses the remaining link's read-only attribute on the original implementation and preserves it with the patch.
  • Both direct/recursive regression cases fail when the original utils.remove implementation is loaded from HEAD into the test process, and pass with the patch. This comparison does not replace the entire installed package with an unchanged baseline.
  • Supplemental pytest run: 78 passed, 4 failed, 1 skipped, 2 xfailed. All four failures originate in existing symlink creation with WinError 1314 (missing privilege); the new tests pass. Command: python -m pytest tests --ignore=tests/test_reflink.py --ignore=tests/benchmarks -q -o asyncio_default_fixture_loop_scope=function --tb=short with a fresh local --basetemp.
  • Ruff 0.14.11 check and format check pass on the four modified Python files. Mypy 1.19.1 passes all 30 source/test files for both --platform win32 and --platform linux. git diff --check passes.
  • End-to-end local CLI comparisons used DVC 3.67.1 and dvc-data 3.18.3 on Python 3.12.15, with the full dvc-objects source from base 32edb41 or this patch imported into the same environment. In fresh NTFS repositories, both dvc checkout --relink and dvc unprotect left the shared cache object writable on the base, and a subsequent workspace append corrupted its contents. Both commands preserved cache protection with the patch, and the append was blocked. The unprotect case also verified that the selected file became a writable independent copy while the second workspace hardlink and cache stayed protected.

Full Nox setup stopped at its native reflink test dependency, which requires Microsoft C++ build tools unavailable locally. Full Nox and full pre-commit remain deferred.

Refresh in 43d80cb:

  • Merged upstream main d3ad931, including its pytest-asyncio minimum-version and fixture-loop-scope configuration fix. The contribution diff remains five files and 177 added lines.
  • All 11 focused Windows hardlink and API-failure tests passed with the repository configuration and no command-line loop-scope override.
  • Ruff 0.17.3 check and format check passed on the four modified Python files. Mypy 1.19.1 passed all 30 source/test files for both Windows and Linux targets. The earlier validation above predates this merge.
  • Full local Nox and full pre-commit remain deferred; hosted CI on this refreshed head is pending. No system software was installed.

Test follow-up in 210ef44 simulates Linux and macOS platform detection and checks that callers receive ENOTSUP and retain the original file contents. The 13 focused removal/API cases pass on Windows; the changed test file passes Ruff 0.17.3 and Mypy. The Tests workflow passed on this follow-up: https://github.com/treeverse/dvc-objects/actions/runs/43127057147 . The complete Tests workflow passed on the preceding merge 43d80cb.

AI assistance: OpenAI Codex investigated the issue, prepared the implementation and tests, and ran the checks above.

@github-project-automation github-project-automation Bot moved this to Backlog in DVC Nov 5, 2026
@codecov

codecov Bot commented Nov 5, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.32143% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.67%. Comparing base (1742874) to head (210ef44).
⚠️ Report is 66 commits behind head on main.

Files with missing lines Patch % Lines
tests/fs/test_utils.py 96.07% 1 Missing and 1 partial ⚠️
src/dvc_objects/fs/utils.py 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #381      +/-   ##
==========================================
+ Coverage   69.53%   73.67%   +4.14%     
==========================================
  Files          26       26              
  Lines        1940     2082     +142     
  Branches      319      196     -123     
==========================================
+ Hits         1349     1534     +185     
+ Misses        553      504      -49     
- Partials       38       44       +6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@CLAassistant

CLAassistant commented Nov 5, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@karthiksenv
karthiksenv marked this pull request as ready for review November 6, 2026 04:03

This branch has not been deployed

No deployments
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