From 001103f688940775c1cf2840ddeea65317133b0d Mon Sep 17 00:00:00 2001 From: Jon Froehlich Date: Wed, 1 Jul 2026 12:55:18 -0700 Subject: [PATCH 1/2] Derive log file path from BASE_DIR, degrade gracefully if unwritable (#1283) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The LOGGING 'file' handler hardcoded an absolute, container-specific path (/code/media/debug.log). Django evaluates LOGGING at django.setup(), so on any host lacking that exact directory (e.g. GitHub Actions CI) startup died with FileNotFoundError before a single request or test ran. Derive the path from BASE_DIR instead (still under media/ so it stays reachable via the intentional /logs/ URL per docs/DEPLOYMENT.md), allow an ML_LOG_DIR env override, and fall back to a NullHandler when the log dir can't be created or written so a bad log path never crashes startup. In the container/servers BASE_DIR is /code, so the default still resolves to /code/media/debug.log — no behavior change on prod, test, or local dev. The NullHandler swap in settings_test.py is now belt-and-suspenders rather than the only crash guard. Co-Authored-By: Claude Opus 4.8 (1M context) --- makeabilitylab/settings.py | 36 ++++++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/makeabilitylab/settings.py b/makeabilitylab/settings.py index b82d0ec8..ee3585bf 100644 --- a/makeabilitylab/settings.py +++ b/makeabilitylab/settings.py @@ -105,6 +105,23 @@ # See: https://docs.djangoproject.com/en/2.0/topics/logging/ # https://lincolnloop.com/blog/django-logging-right-way/ # For the log format, see: https://stackoverflow.com/a/26276689/388117 +# +# Log-file path (issue #1283): this used to be hardcoded to /code/media/debug.log, +# an absolute container-specific path. Django evaluates LOGGING at django.setup(), +# so on any host lacking that exact directory (e.g. GitHub Actions CI) startup died +# with FileNotFoundError before a single request/test ran. Derive the path from +# BASE_DIR instead (still under media/ so it stays reachable via the intentional +# /logs/ URL — see docs/DEPLOYMENT.md), allow an ML_LOG_DIR env override, and if +# the directory can't be created or written, fall back to a NullHandler so a bad +# log path never crashes startup. +LOG_DIR = os.environ.get('ML_LOG_DIR', os.path.join(BASE_DIR, 'media')) +LOG_FILE = os.path.join(LOG_DIR, 'debug.log') +try: + os.makedirs(LOG_DIR, exist_ok=True) + _LOG_TO_FILE = os.access(LOG_DIR, os.W_OK) +except OSError: + _LOG_TO_FILE = False + LOGGING = { 'version': 1, 'disable_existing_loggers': False, @@ -125,20 +142,23 @@ }, }, 'handlers': { + # The file handler writes LOG_FILE (media/debug.log by default), which + # lands in the bind-mounted web root and is intentionally exposed via the + # /logs/ URL per docs/DEPLOYMENT.md (Jason Howe's design — convenient + # remote debugging in exchange for some info disclosure). To shrink that + # exposure in production, we log at INFO when DEBUG is off, but keep + # DEBUG-level file logging in local dev where DEBUG is on and the file + # isn't publicly reachable. If the log dir isn't writable (_LOG_TO_FILE + # is False), degrade to a NullHandler so startup never dies (issue #1283). 'file': { - # The file handler writes /code/media/debug.log, which lands in the - # bind-mounted web root and is intentionally exposed via the /logs/ - # URL per docs/DEPLOYMENT.md (Jason Howe's design — convenient - # remote debugging in exchange for some info disclosure). To shrink - # that exposure in production, we log at INFO when DEBUG is off, - # but keep DEBUG-level file logging in local dev where DEBUG is on - # and the file isn't publicly reachable. 'level': 'DEBUG' if DEBUG else 'INFO', 'class': 'logging.handlers.RotatingFileHandler', - 'filename': '/code/media/debug.log', + 'filename': LOG_FILE, 'maxBytes': 1024*1024*5, # 5 MB 'backupCount': 6, 'formatter': 'verbose', # can switch between verbose and simple + } if _LOG_TO_FILE else { + 'class': 'logging.NullHandler', }, 'console': { 'level': 'DEBUG', From ce9ecf518de3e6e1013592084ee89e8c25fa399c Mon Sep 17 00:00:00 2001 From: Jon Froehlich Date: Wed, 8 Jul 2026 18:01:54 -0700 Subject: [PATCH 2/2] Extract log-dir writability guard into a tested helper (#1283) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback on the log-path degradation fix: - Pull the try/except guard out of the LOGGING block into a documented _log_dir_is_writable() helper so it can be unit-tested directly (no settings reload / subprocess needed). - Note the dir-vs-file writability gap in the helper docstring: the check is on the directory, so a writable dir holding a root-owned read-only debug.log could still make RotatingFileHandler raise on open — an accepted edge case, strictly better than the prior unconditional crash. - Add website/tests/test_logging_config.py: a fast SimpleTestCase pinning both the writable (→ file handler) and uncreatable (→ NullHandler) paths. Co-Authored-By: Claude Opus 4.8 (1M context) --- makeabilitylab/settings.py | 25 ++++++++++++++---- website/tests/test_logging_config.py | 39 ++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 5 deletions(-) create mode 100644 website/tests/test_logging_config.py diff --git a/makeabilitylab/settings.py b/makeabilitylab/settings.py index ee3585bf..d889d29a 100644 --- a/makeabilitylab/settings.py +++ b/makeabilitylab/settings.py @@ -114,13 +114,28 @@ # /logs/ URL — see docs/DEPLOYMENT.md), allow an ML_LOG_DIR env override, and if # the directory can't be created or written, fall back to a NullHandler so a bad # log path never crashes startup. +def _log_dir_is_writable(log_dir): + """Return True if ``log_dir`` exists (or can be created) and looks writable. + + Used to decide whether the file log handler is active or degrades to a + NullHandler so a bad log path never crashes ``django.setup()`` (issue #1283). + + Note: this checks the *directory*, not the eventual log file. A dir that is + writable but already holds a root-owned, read-only ``debug.log`` would still + let RotatingFileHandler raise on open — an edge case we accept, since it is + strictly better than the previous unconditional crash and matches the real + deploy model (media/ is owned by the app's own user). + """ + try: + os.makedirs(log_dir, exist_ok=True) + return os.access(log_dir, os.W_OK) + except OSError: + return False + + LOG_DIR = os.environ.get('ML_LOG_DIR', os.path.join(BASE_DIR, 'media')) LOG_FILE = os.path.join(LOG_DIR, 'debug.log') -try: - os.makedirs(LOG_DIR, exist_ok=True) - _LOG_TO_FILE = os.access(LOG_DIR, os.W_OK) -except OSError: - _LOG_TO_FILE = False +_LOG_TO_FILE = _log_dir_is_writable(LOG_DIR) LOGGING = { 'version': 1, diff --git a/website/tests/test_logging_config.py b/website/tests/test_logging_config.py new file mode 100644 index 00000000..8161bd47 --- /dev/null +++ b/website/tests/test_logging_config.py @@ -0,0 +1,39 @@ +""" +Regression tests for the log-path degradation guard added in issue #1283. + +The base ``LOGGING`` config used to hardcode ``/code/media/debug.log``. Because +Django evaluates ``LOGGING`` at ``django.setup()``, any host missing that exact +directory (e.g. GitHub Actions CI) crashed with ``FileNotFoundError`` before a +single request or test ran. The fix derives the path from ``BASE_DIR`` and, if +the log directory can't be created or written, degrades the file handler to a +``NullHandler`` so startup never dies. These tests pin that helper's behavior. + +Pure logic, no DB — a fast ``SimpleTestCase``. +""" + +import shutil +import tempfile + +from django.test import SimpleTestCase + +from makeabilitylab.settings import _log_dir_is_writable + + +class LogDirWritabilityTests(SimpleTestCase): + def test_writable_dir_returns_true(self): + """A normal, writable directory keeps the file handler active.""" + tmp = tempfile.mkdtemp() + try: + self.assertTrue(_log_dir_is_writable(tmp)) + finally: + shutil.rmtree(tmp, ignore_errors=True) + + def test_uncreatable_dir_returns_false(self): + """A dir that can't be created degrades to False (→ NullHandler). + + ``/dev/null`` is a file on every POSIX host, so ``os.makedirs`` under it + raises ``NotADirectoryError`` (an ``OSError``) — the helper must swallow + it and report the directory as unusable rather than letting the error + propagate into ``django.setup()``. + """ + self.assertFalse(_log_dir_is_writable('/dev/null/cannot/create'))