From 9a8ed8d3fb12cfeb9c195bd1d22714be51d044cd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 10 Mar 2026 21:03:22 +0000 Subject: [PATCH 1/4] Initial plan From 32af264d1c5174963760852696ca22fe2018d83d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 10 Mar 2026 21:06:06 +0000 Subject: [PATCH 2/4] Fix inter-process locking, email key restriction, dict check, and conditional settings save Co-authored-by: dvir001 <39403717+dvir001@users.noreply.github.com> --- config/app_settings.json.lock | 0 simple_org_chart/app_main.py | 5 ++- simple_org_chart/email_config.py | 22 ++++++---- simple_org_chart/settings.py | 74 ++++++++++++++++++++++++++++++-- 4 files changed, 89 insertions(+), 12 deletions(-) create mode 100644 config/app_settings.json.lock diff --git a/config/app_settings.json.lock b/config/app_settings.json.lock new file mode 100644 index 0000000..e69de29 diff --git a/simple_org_chart/app_main.py b/simple_org_chart/app_main.py index a54b0da..3bc002b 100644 --- a/simple_org_chart/app_main.py +++ b/simple_org_chart/app_main.py @@ -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: diff --git a/simple_org_chart/email_config.py b/simple_org_chart/email_config.py index 57ef6aa..5b0c2e3 100644 --- a/simple_org_chart/email_config.py +++ b/simple_org_chart/email_config.py @@ -61,14 +61,18 @@ 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() + merged = DEFAULT_EMAIL_CONFIG.copy() + for key in _EMAIL_KEYS: + if key in stored: + merged[key] = stored[key] + return merged + return DEFAULT_EMAIL_CONFIG.copy() @@ -76,9 +80,11 @@ 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) + for key in _EMAIL_KEYS: + if key in config: + persisted[key] = config[key] if 'lastSent' not in config: # lastSent will be preserved from the existing file inside the lock below diff --git a/simple_org_chart/settings.py b/simple_org_chart/settings.py index 67763d1..ee51441 100644 --- a/simple_org_chart/settings.py +++ b/simple_org_chart/settings.py @@ -13,10 +13,78 @@ 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 + try: + import fcntl + fd = os.open(self._lock_file_path, os.O_CREAT | os.O_RDWR, 0o600) + self._fd = fd + flags = fcntl.LOCK_EX | (0 if blocking else fcntl.LOCK_NB) + fcntl.flock(fd, flags) + except ImportError: + # fcntl not available (Windows); thread lock is sufficient. + pass + except BlockingIOError: + self._thread_lock.release() + return False + except Exception: + 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: + if self._thread_lock.locked(): + self._thread_lock.release() + + def __enter__(self) -> "_InterProcessSettingsFileLock": + self.acquire() + 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__) From d5dc3947f6f9a464ef2c37391f7bfe84517689ff Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 10 Mar 2026 21:06:30 +0000 Subject: [PATCH 3/4] Remove committed lock file and add config/*.lock to .gitignore Co-authored-by: dvir001 <39403717+dvir001@users.noreply.github.com> --- .gitignore | 3 +++ config/app_settings.json.lock | 0 2 files changed, 3 insertions(+) delete mode 100644 config/app_settings.json.lock diff --git a/.gitignore b/.gitignore index 0bb3ba4..0554d6e 100644 --- a/.gitignore +++ b/.gitignore @@ -7,3 +7,6 @@ repositories/ __pycache__/ *.pyc flask_session/ + +# Runtime lock files +config/*.lock diff --git a/config/app_settings.json.lock b/config/app_settings.json.lock deleted file mode 100644 index e69de29..0000000 From ac9f7d55706e64a200af9726b058ca63724c35fb Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 10 Mar 2026 21:08:35 +0000 Subject: [PATCH 4/4] Fix fd leak in lock acquire, remove locked() race, and extract _filter_email_keys helper Co-authored-by: dvir001 <39403717+dvir001@users.noreply.github.com> --- simple_org_chart/email_config.py | 14 ++++++++------ simple_org_chart/settings.py | 13 +++++++++---- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/simple_org_chart/email_config.py b/simple_org_chart/email_config.py index 5b0c2e3..b4019ab 100644 --- a/simple_org_chart/email_config.py +++ b/simple_org_chart/email_config.py @@ -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) @@ -68,9 +74,7 @@ def load_email_config() -> Dict[str, Any]: logger.warning("app_settings.json does not contain a JSON object; using email defaults") return DEFAULT_EMAIL_CONFIG.copy() merged = DEFAULT_EMAIL_CONFIG.copy() - for key in _EMAIL_KEYS: - if key in stored: - merged[key] = stored[key] + merged.update(_filter_email_keys(stored)) return merged return DEFAULT_EMAIL_CONFIG.copy() @@ -82,9 +86,7 @@ def save_email_config(config: Dict[str, Any]) -> bool: # Restrict to known email keys only — never let caller overwrite unrelated settings persisted = DEFAULT_EMAIL_CONFIG.copy() - for key in _EMAIL_KEYS: - if key in config: - persisted[key] = config[key] + persisted.update(_filter_email_keys(config)) if 'lastSent' not in config: # lastSent will be preserved from the existing file inside the lock below diff --git a/simple_org_chart/settings.py b/simple_org_chart/settings.py index ee51441..2913637 100644 --- a/simple_org_chart/settings.py +++ b/simple_org_chart/settings.py @@ -33,19 +33,25 @@ 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) - self._fd = fd flags = fcntl.LOCK_EX | (0 if blocking else fcntl.LOCK_NB) fcntl.flock(fd, flags) + self._fd = fd except ImportError: # fcntl not available (Windows); thread lock is sufficient. - pass + 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 @@ -65,8 +71,7 @@ def release(self) -> None: pass self._fd = None finally: - if self._thread_lock.locked(): - self._thread_lock.release() + self._thread_lock.release() def __enter__(self) -> "_InterProcessSettingsFileLock": self.acquire()