Revert Windows private-file ACL restriction (broke main) - #158
Merged
Conversation
…user" This reverts 576ed69. It broke the Windows lifecycle job on main: sqlite3.OperationalError: attempt to write a readonly database core\persistence\database.py:160 _restrict_windows_acl() runs two icacls calls in sequence: 1. /inheritance:r strips every inherited ACE 2. /grant:r <user>:F re-grants the current user The sequence is not atomic, and the except branch returns without undoing step 1. If the grant fails or has not taken effect, the file is left with no usable ACE at all — every caller of private_storage, including the SQLite database, then fails closed. That is not the best-effort degradation the docstring claims. It is also intermittent: the same tree passed this job on the PR branch and failed on main, so a single green run does not clear it. Also drops tests/test_private_storage_windows.py from the Windows CI step, since the revert removes that file. The fix is to grant before stripping inheritance, and to leave inheritance alone when the grant fails — worst case then is permissions not tightened, rather than a file nothing can open. That will come back as its own PR with repeated Windows runs to cover the intermittency. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
576ed69(from #148, merged in #156) broke the Windows lifecycle job onmain:Root cause
_restrict_windows_acl()runs twoicaclscalls in sequence:/inheritance:r— strips every inherited ACE/grant:r <user>:F— re-grants the current userThe sequence is not atomic, and the
exceptbranch returns without undoing step 1. If the grant fails or has not taken effect, the file is left with no usable ACE at all — every caller ofprivate_storage, including the SQLite database, then fails closed. That is not the best-effort degradation the docstring claims.It is also intermittent: the same tree passed this job on the PR branch and failed on
main. A single green run does not clear it.Scope
576ed69(core/private_storage.pyand its test file)tests/test_private_storage_windows.pyfrom the Windows CI step, since the revert removes that fileEverything else from #156 stays: the
ensure_asciifix (#139), the Job Object sandbox backend (#149), and the Windows CI coverage.Follow-up
The fix is to grant before stripping inheritance, and to leave inheritance alone when the grant fails — worst case then is permissions not tightened, rather than a file nothing can open. That will come back as its own PR, with repeated Windows runs to cover the intermittency.
@raymondginger2018-sudo — your #149 is unaffected and stays in. Sorry for the churn on #148; the idea is right, the ordering just needs to fail safe.