Bugfix/issue 87 permissions race#88
Merged
Preston-Landers merged 5 commits intomasterfrom Apr 14, 2026
Merged
Conversation
ConcurrentRotatingFileHandler created files using the configured umask and called _do_chown_and_chmod() afterward to apply the target perms. Between create and chmod, the file was visible in the filesystem with umask-derived (potentially too-restrictive) permissions. A different- user process that opened the file during this window got PermissionError, breaking cross-user log sharing — the exact scenario the chmod/owner kwargs exist to support. Three call sites had this pattern: 1. _open_lockfile(): the per-handler ".<name>.lock" file. 2. do_open(): the main log file. 3. doRollover(): the ".1.gz" file produced during gzip rotation. Fix: add _atomic_create_with_perms(), which creates a tempfile in the same directory, applies chown/chmod to it, then atomically links it into place via os.link() (POSIX) or os.rename() (Windows). Other processes never observe the public filename with intermediate perms — either they see it with correct perms, or not at all. The helper is gated on (chmod is not None or owner is not None) AND (target does not already exist), so handlers that don't configure permissions pay zero extra syscalls. Existing _do_chown_and_chmod() calls are preserved as idempotent safety nets. For the gzip rotation path, the fix is slightly different: we apply chown/chmod to the temp ".gz" file *before* it gets renamed into ".1.gz", so the public name never appears with wrong perms. Also fix a related bug uncovered while investigating: do_gzip() silently ignored the configured umask kwarg because gzip.open() ran outside any _alter_umask() context, so rotated .gz files picked up the process default umask instead of the handler's. Now wrapped. Add a non-root regression test (tests/test_perm_race.py) that monkey-patches _do_chown_and_chmod to spy on the file's on-disk mode at call time. Asserts that the publicly-visible target paths are never observed with the wrong mode. Covers all three call sites. Thanks to @jbfryar for a thorough bug report on this issue.
Mypy 1.20 dropped Python 3.8 as a static-check target and tightened detection of unused "type: ignore" comments. With mypy unpinned in the lint env, fresh CI runs picked up 1.20.1 and started failing on pre-existing issues. - Remove two now-unused "type: ignore[assignment]" comments on `self.stream = None` assignments. The logging.Handler stub no longer needs the suppression. - Bump [tool.mypy] python_version from "3.8" to "3.9". Runtime support for Python 3.6+ is unchanged; this only affects static type-checking. - Pin mypy to ">=1.10.0,<2" in the lint env so future major releases are an explicit decision rather than a surprise CI break.
Recent versions of hatch + virtualenv on the GitHub runner fail to
create environments for older Pythons with:
module 'virtualenv.discovery.builtin' has no attribute 'propose_interpreters'
This is upstream tooling drift, not a project issue, but it broke the
3.7, 3.8, and 3.9 matrix entries.
We still want to keep these EOL versions in the test matrix as a
canary on whether the library itself works on them. Split the test
job's run path by Python version: 3.10+ continues to use hatch as
before, while 3.7/3.8/3.9 install the package + test deps directly
with pip and run pytest/coverage directly. The dev extras include
black>=26.1.0 (Python 3.9+ only), so the legacy entries install only
the test-relevant deps explicitly.
The legacy version list is encoded once per branch via
`contains(fromJSON('["3.7", "3.8", "3.9"]'), matrix.python-version)`,
so adding/removing a version when upstream drift claims more
interpreters is a one-place edit per condition rather than a fan-out
of `||` chains.
Lint/typing checks still run once on the Python 3.14 lint job, so
nothing is lost from skipping them for the legacy entries.
Three small workflow updates:
- Add `workflow_dispatch:` to the workflow's `on:` triggers. Provides
a "Run workflow" button in the Actions UI for re-running CI against
a branch without needing to push an empty commit.
- Bump action versions to the Node 24-compatible majors:
actions/checkout@v3, @v4 -> @v5
actions/setup-python@v4 -> @v6
Clears the recent deprecation warning about Node 20 actions being
removed from runners in September 2026. No behavior change in the
way these actions are used here.
- Drop coverage collection from the legacy Python (3.7/3.8/3.9) test
path. Tests pass but the step exited 1 because `coverage report`
found "No data to report." -- `coverage run -m pytest` together with
pytest-cov produces an empty `.coverage` in some plugin orderings.
Coverage is fully collected on the modern (hatch) matrix entries,
so the legacy path now just runs `pytest tests` and only verifies
that the library itself works on EOL Pythons. Removed `coverage`
and `pytest-cov` from the legacy install line accordingly.
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.
See: #87