Skip to content

fix: apply UMASK env var via os.umask() at Python startup#111

Merged
nitrobass24 merged 2 commits intomasterfrom
fix/umask-not-applied
Mar 2, 2026
Merged

fix: apply UMASK env var via os.umask() at Python startup#111
nitrobass24 merged 2 commits intomasterfrom
fix/umask-not-applied

Conversation

@nitrobass24
Copy link
Owner

@nitrobass24 nitrobass24 commented Mar 2, 2026

Summary

  • The shell umask set in entrypoint.sh is not reliably inherited by the lftp subprocess through the setpriv exec chain in all container environments
  • Downloaded files were created with the system default umask (00020664) instead of the user-configured value
  • Fix reads UMASK env var directly in Python and calls os.umask() at startup, before pexpect spawns lftp

Fixes #109

Test plan

  • Set UMASK=000 in Docker environment variables
  • Queue a file for download
  • Verify downloaded files have 0666 permissions (rw-rw-rw-) instead of 0664
  • Verify downloaded directories have 0777 permissions
  • Verify that omitting UMASK (empty string, the default) leaves system umask unchanged

Summary by CodeRabbit

  • New Features
    • Added support for configuring process umask via an environment variable; the value is applied at startup and a warning is emitted for invalid values.

The shell umask set in entrypoint.sh via `umask "$UMASK"` was not being
reliably inherited by the lftp subprocess (spawned via pexpect) through
the setpriv exec chain in all container environments. As a result,
downloaded files were created with the system default umask (0002 →
0664) instead of the user-configured value.

Fix by reading the UMASK env var directly in Python and calling
os.umask() before any child processes are spawned, ensuring lftp
inherits the correct file creation mask.

Fixes #109

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link

coderabbitai bot commented Mar 2, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1ddd7bb and cc8dead.

📒 Files selected for processing (1)
  • src/python/seedsync.py

📝 Walkthrough

Walkthrough

Adds UMASK handling at startup: reads UMASK from the environment, attempts octal parsing, applies it via os.umask(), and prints a stderr warning on invalid values. This runs before spawning child processes and does not change other control flow.

Changes

Cohort / File(s) Summary
UMASK Configuration
src/python/seedsync.py
Read UMASK env var, parse as octal, call os.umask() before creating child processes; prints a stderr warning if the value is invalid.

Sequence Diagram(s)

sequenceDiagram
    participant Env as Environment
    participant Main as Main process
    participant Child as Child processes

    Env->>Main: UMASK value (or absent)
    Main->>Main: parse UMASK as octal (try/except)
    alt valid UMASK
        Main->>Main: call os.umask(parsed_value)
    else invalid UMASK
        Main->>Main: print warning to stderr
    end
    Main->>Child: spawn child processes (inherit umask)
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Poem

🐰
I sniffed the env for a tiny mask,
Parsed the digits, a careful task.
If odd bits show, I whisper a scold,
Then set the tone before young processes unfold.
Hop — permissions tidy, neat, and bold. 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'fix: apply UMASK env var via os.umask() at Python startup' accurately and specifically describes the main change: reading and applying the UMASK environment variable in Python before child process spawning.
Linked Issues check ✅ Passed The code changes address the core requirement from issue #109 by implementing os.umask() in Python at startup, ensuring child processes inherit the configured UMASK before lftp is spawned via pexpect.
Out of Scope Changes check ✅ Passed The changes are strictly scoped to the required functionality: reading UMASK from environment, parsing it as octal, applying via os.umask(), and handling invalid values with a stderr warning.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/umask-not-applied

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/python/seedsync.py`:
- Line 412: Replace the .format-based print call in seedsync.py with an
f-string: locate the print call that emits the warning for an invalid UMASK (the
line using "_umask_str" and print(..., file=sys.stderr)) and rewrite the message
to use an f-string (e.g., f"WARNING: Invalid UMASK value {_umask_str!r},
ignoring") while preserving the target (sys.stderr) and surrounding behavior.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6b586f2 and 1ddd7bb.

📒 Files selected for processing (1)
  • src/python/seedsync.py

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@nitrobass24 nitrobass24 merged commit 0b18669 into master Mar 2, 2026
11 checks passed
@nitrobass24 nitrobass24 deleted the fix/umask-not-applied branch March 2, 2026 19:52
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.

0.12.6 > UMASK is not applying correct permissions on DL'd files

1 participant