Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,6 @@ repositories/
__pycache__/
*.pyc
flask_session/

# Runtime lock files
config/*.lock
5 changes: 4 additions & 1 deletion simple_org_chart/app_main.py
Original file line number Diff line number Diff line change
Expand Up @@ -1200,7 +1200,10 @@ def import_settings():
elif key in email_keys:
email_data[key] = value

settings_ok = save_settings(settings_data)
# Only save settings if at least one valid settings key was provided
settings_ok = True
if settings_data:
settings_ok = save_settings(settings_data)

email_ok = True
if email_data:
Expand Down
24 changes: 16 additions & 8 deletions simple_org_chart/email_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@
# Keys that belong to the email configuration
_EMAIL_KEYS = set(DEFAULT_EMAIL_CONFIG.keys())


def _filter_email_keys(source: Dict[str, Any]) -> Dict[str, Any]:
"""Return a new dict containing only keys that belong to the email config."""
return {key: source[key] for key in _EMAIL_KEYS if key in source}


def get_smtp_config() -> Dict[str, Any]:
"""Load SMTP configuration from environment variables."""
# Get encryption setting (TLS, SSL, or None)
Expand Down Expand Up @@ -61,24 +67,26 @@ def load_email_config() -> Dict[str, Any]:
try:
with SETTINGS_FILE.open("r", encoding="utf-8") as handle:
stored = json.load(handle)
merged = DEFAULT_EMAIL_CONFIG.copy()
for key in _EMAIL_KEYS:
if key in stored:
merged[key] = stored[key]
return merged
except Exception as error:
logger.error("Error loading email config: %s", error)

else:
if not isinstance(stored, dict):
logger.warning("app_settings.json does not contain a JSON object; using email defaults")
return DEFAULT_EMAIL_CONFIG.copy()
Comment on lines +73 to +75
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

The warning hard-codes app_settings.json. If SETTINGS_FILE is ever renamed or configured differently, this message becomes misleading. Prefer including the actual SETTINGS_FILE (path or name) in the log message.

Copilot uses AI. Check for mistakes.
merged = DEFAULT_EMAIL_CONFIG.copy()
merged.update(_filter_email_keys(stored))
return merged

return DEFAULT_EMAIL_CONFIG.copy()


def save_email_config(config: Dict[str, Any]) -> bool:
"""Save email configuration into app_settings.json."""
SETTINGS_FILE.parent.mkdir(parents=True, exist_ok=True)

# Merge with defaults
# Restrict to known email keys only — never let caller overwrite unrelated settings
persisted = DEFAULT_EMAIL_CONFIG.copy()
persisted.update(config)
persisted.update(_filter_email_keys(config))

if 'lastSent' not in config:
# lastSent will be preserved from the existing file inside the lock below
Expand Down
79 changes: 76 additions & 3 deletions simple_org_chart/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,83 @@

from .config import SETTINGS_FILE

# Shared in-process lock protecting all reads/writes to SETTINGS_FILE.

class _InterProcessSettingsFileLock:
"""Combined thread + process lock for protecting SETTINGS_FILE.

Serialises concurrent access across both threads within a single worker
and across multiple Gunicorn worker processes by combining a
``threading.Lock`` with an ``fcntl.flock`` advisory file lock. On
platforms where ``fcntl`` is unavailable (e.g. Windows) the class
falls back gracefully to thread-only locking.
"""

def __init__(self, lock_file_path: str) -> None:
self._lock_file_path = lock_file_path
self._thread_lock = threading.Lock()
self._fd: int | None = None

def acquire(self, blocking: bool = True) -> bool:
acquired = self._thread_lock.acquire(blocking)
if not acquired:
return False
fd = None
try:
import fcntl
fd = os.open(self._lock_file_path, os.O_CREAT | os.O_RDWR, 0o600)
flags = fcntl.LOCK_EX | (0 if blocking else fcntl.LOCK_NB)
fcntl.flock(fd, flags)
Comment on lines +38 to +41
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

os.open(self._lock_file_path, ...) will raise FileNotFoundError if the parent directory doesn’t exist (e.g., first run / before any code path calls SETTINGS_FILE.parent.mkdir(...)). Since acquiring the lock can happen during read paths, this can crash callers. Create the lock file’s parent directory inside acquire() (or during initialization) before calling os.open.

Copilot uses AI. Check for mistakes.
self._fd = fd
except ImportError:
# fcntl not available (Windows); thread lock is sufficient.
if fd is not None:
os.close(fd)
except BlockingIOError:
if fd is not None:
os.close(fd)
self._thread_lock.release()
return False
except Exception:
if fd is not None:
os.close(fd)
self._thread_lock.release()
raise
return True

def release(self) -> None:
try:
if self._fd is not None:
try:
import fcntl
fcntl.flock(self._fd, fcntl.LOCK_UN)
except (ImportError, OSError):
pass
finally:
try:
os.close(self._fd)
except OSError:
pass
self._fd = None
finally:
self._thread_lock.release()

def __enter__(self) -> "_InterProcessSettingsFileLock":
self.acquire()
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

__enter__ ignores the return value of acquire(). While with lock: currently calls the blocking path, this makes the class easier to misuse (e.g., if acquire(blocking=False) behavior is later introduced into context-manager usage). Consider raising an exception if acquire() returns False, or changing acquire() to always raise on failure so the context manager can’t enter without owning the lock.

Suggested change
self.acquire()
acquired = self.acquire()
if not acquired:
raise RuntimeError("Failed to acquire settings file lock")

Copilot uses AI. Check for mistakes.
return self

def __exit__(self, exc_type: Any, exc_val: Any, exc_tb: Any) -> None:
self.release()


# Shared inter-process lock protecting all reads/writes to SETTINGS_FILE.
# Both settings.py and email_config.py import this lock so that concurrent
# writes from different code paths (scheduler, HTTP handlers) are serialised.
_settings_file_lock = threading.Lock()
# writes from different code paths (scheduler, HTTP handlers, workers) are
# serialised across all Gunicorn worker processes.
_settings_lock_file = os.path.join(
os.fspath(SETTINGS_FILE.parent),
f"{SETTINGS_FILE.name}.lock",
)
_settings_file_lock = _InterProcessSettingsFileLock(_settings_lock_file)

logger = logging.getLogger(__name__)

Expand Down
Loading