fix: catch OSError from SETTINGS_FILE.exists() — Docker UID-mismatch 500 crash (#614)
Squash-merges PR #614. Fixes Docker 500-on-every-request crash from PermissionError in load_settings() (issue #570 follow-up). Both SETTINGS_FILE.exists() call sites now catch OSError and fall back to defaults. Reviewer nits addressed: removed unused imports/var in tests, improved log message to say "inaccessible?" instead of "permission denied?". Rebased clean onto v0.50.73. 1373 tests passing, QA harness green.
This commit is contained in:
@@ -1232,7 +1232,14 @@ _SETTINGS_LEGACY_DROP_KEYS = {"assistant_language"}
|
|||||||
def load_settings() -> dict:
|
def load_settings() -> dict:
|
||||||
"""Load settings from disk, merging with defaults for any missing keys."""
|
"""Load settings from disk, merging with defaults for any missing keys."""
|
||||||
settings = dict(_SETTINGS_DEFAULTS)
|
settings = dict(_SETTINGS_DEFAULTS)
|
||||||
if SETTINGS_FILE.exists():
|
try:
|
||||||
|
settings_exists = SETTINGS_FILE.exists()
|
||||||
|
except OSError:
|
||||||
|
# PermissionError or other OS-level error (e.g. UID mismatch in Docker)
|
||||||
|
# Treat as missing — start with defaults rather than crashing.
|
||||||
|
logger.debug("Cannot stat settings file %s (inaccessible?)", SETTINGS_FILE)
|
||||||
|
settings_exists = False
|
||||||
|
if settings_exists:
|
||||||
try:
|
try:
|
||||||
stored = json.loads(SETTINGS_FILE.read_text(encoding="utf-8"))
|
stored = json.loads(SETTINGS_FILE.read_text(encoding="utf-8"))
|
||||||
if isinstance(stored, dict):
|
if isinstance(stored, dict):
|
||||||
@@ -1316,7 +1323,11 @@ def save_settings(settings: dict) -> dict:
|
|||||||
# config must never shadow an explicit env-var override (Docker deployments
|
# config must never shadow an explicit env-var override (Docker deployments
|
||||||
# rely on this — otherwise deleting settings.json is the only escape).
|
# rely on this — otherwise deleting settings.json is the only escape).
|
||||||
_startup_settings = load_settings()
|
_startup_settings = load_settings()
|
||||||
if SETTINGS_FILE.exists():
|
try:
|
||||||
|
_settings_file_exists = SETTINGS_FILE.exists()
|
||||||
|
except OSError:
|
||||||
|
_settings_file_exists = False
|
||||||
|
if _settings_file_exists:
|
||||||
if _startup_settings.get("default_model"):
|
if _startup_settings.get("default_model"):
|
||||||
DEFAULT_MODEL = _startup_settings["default_model"]
|
DEFAULT_MODEL = _startup_settings["default_model"]
|
||||||
if not os.getenv("HERMES_WEBUI_DEFAULT_WORKSPACE"):
|
if not os.getenv("HERMES_WEBUI_DEFAULT_WORKSPACE"):
|
||||||
|
|||||||
56
tests/test_issue570_permission.py
Normal file
56
tests/test_issue570_permission.py
Normal file
@@ -0,0 +1,56 @@
|
|||||||
|
"""
|
||||||
|
Regression tests for GitHub issue #570 follow-up:
|
||||||
|
PermissionError from SETTINGS_FILE.exists() in Docker UID-mismatch scenarios.
|
||||||
|
|
||||||
|
When ~/.hermes is owned by a different UID than the container user (common in
|
||||||
|
Docker setups), Path.exists() raises PermissionError instead of returning False.
|
||||||
|
load_settings() must treat that as "file not accessible = use defaults" rather
|
||||||
|
than propagating the exception up to crash the request handler.
|
||||||
|
"""
|
||||||
|
import stat
|
||||||
|
import pytest
|
||||||
|
import api.config as config
|
||||||
|
|
||||||
|
|
||||||
|
def test_load_settings_returns_defaults_when_settings_file_unreadable(monkeypatch, tmp_path):
|
||||||
|
"""PermissionError from SETTINGS_FILE.exists() must not propagate — return defaults instead.
|
||||||
|
|
||||||
|
Regression for issue #570 comment: Docker UID mismatch caused every request
|
||||||
|
to 500 because load_settings() called SETTINGS_FILE.exists() without catching OSError.
|
||||||
|
"""
|
||||||
|
state_dir = tmp_path / "state"
|
||||||
|
state_dir.mkdir()
|
||||||
|
settings_file = state_dir / "settings.json"
|
||||||
|
# Create the file then make the parent unreadable so .exists() raises PermissionError
|
||||||
|
settings_file.write_text('{"send_key": "ctrl+enter"}', encoding="utf-8")
|
||||||
|
state_dir.chmod(stat.S_IWUSR) # write-only: stat() on children will fail
|
||||||
|
|
||||||
|
monkeypatch.setattr(config, "SETTINGS_FILE", settings_file)
|
||||||
|
|
||||||
|
try:
|
||||||
|
result = config.load_settings()
|
||||||
|
# Must not raise; must return a dict with default values
|
||||||
|
assert isinstance(result, dict)
|
||||||
|
assert "send_key" in result
|
||||||
|
# The corrupted/inaccessible value should NOT appear — defaults win
|
||||||
|
assert result["send_key"] == config._SETTINGS_DEFAULTS["send_key"]
|
||||||
|
finally:
|
||||||
|
state_dir.chmod(stat.S_IRWXU) # restore for cleanup
|
||||||
|
|
||||||
|
|
||||||
|
def test_load_settings_returns_defaults_when_exists_raises_permission_error(monkeypatch, tmp_path):
|
||||||
|
"""Direct simulation: monkeypatch SETTINGS_FILE.exists to raise PermissionError."""
|
||||||
|
from unittest import mock
|
||||||
|
|
||||||
|
state_dir = tmp_path / "state"
|
||||||
|
state_dir.mkdir()
|
||||||
|
settings_file = state_dir / "settings.json"
|
||||||
|
|
||||||
|
monkeypatch.setattr(config, "SETTINGS_FILE", settings_file)
|
||||||
|
|
||||||
|
with mock.patch.object(type(settings_file), "exists",
|
||||||
|
side_effect=PermissionError("Permission denied")):
|
||||||
|
result = config.load_settings()
|
||||||
|
|
||||||
|
assert isinstance(result, dict)
|
||||||
|
assert result["send_key"] == config._SETTINGS_DEFAULTS["send_key"]
|
||||||
Reference in New Issue
Block a user