fix: a log that cannot be written does not fail the request - #925
Merged
blaipr merged 1 commit intoSep 17, 2026
Merged
Conversation
LoggerBase::update() — the file/syslog receiver — had no guard around its body, and it is the one receiver attached on every request, before the install check and regardless of any config flag. Its three siblings all catch and hand to processException(), and two of them are config-gated as well. Monolog's StreamHandler throws when var/syspass.log cannot be opened or appended to, and notify() is always called after the work it describes, so that exception propagated out of an operation that had already completed. Router turns it into a generic error response, which means an administrator is told a master-password rotation failed when it had already finished — the one outcome that must never be ambiguous. unsetAppLocales() was skipped on that path too. The body moves into a private writeEvent() and update() wraps it the way the siblings do, with a finally so the locale is restored either way. Throwable rather than the siblings' Exception, because a stream failure can surface as an Error and processException() accepts either; and processException() is safe to call from here because logger() writes with a suppressed file_put_contents() and falls back to error_log(), so it does not come back through Monolog. That this receiver is attached unconditionally is left alone: the file log is the one that has to work before the database and the config are usable. The defect was that it could not fail safely, not that it runs.
blaipr
deleted the
fix/a-log-that-cannot-be-written-does-not-fail-the-request
branch
September 17, 2026 01:34
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
LoggerBase::update()— the file/syslog receiver — has no guard around its body. Its three siblingsall have one:
DatabaseHandlercatch (Exception) → processException()isLogEnabled()MailEventcatch (Exception) → processException()isMailEnabled()NotificationEventcatch (Exception) → processException()LoggerBaseSo it is the one receiver that always runs and the one that cannot fail safely.
Monolog's
StreamHandlerthrows whenvar/syspass.logcannot be opened or appended to — a fulldisk, or permissions on
var/.EventDispatcher::notify()is always called after the work itdescribes, so that exception propagates out of an operation that has already completed.
Routercatches it at the top and turns it into a generic error response, which means: theadministrator is told a master-password rotation failed when it had already finished. That is the
one outcome that must never be ambiguous — the whole reason the rotation writes its hash inside the
transaction.
unsetAppLocales()was also skipped on that path, leaving the request's locale set.The change
The body moves into a private
writeEvent(), andupdate()wraps it the way the siblings do, witha
finallyso the locale is restored either way.Two deliberate differences from the siblings:
Throwable, notException— a stream failure can surface as anError, andprocessException()accepts either.processException()is safe to call from here, which is worth stating because it lookscircular:
logger()writes with a suppressedfile_put_contents()and falls back toerror_log(), so it does not come back through Monolog and cannot throw again.Test
testAFailingLoggerDoesNotFailTheRequestmakes the injected logger throw and assertsupdate()returns — and that
unsetAppLocales()still ran, which a plaintry/catcharound the call wouldhave missed.
Mutation-verified: reverting
src/lets theRuntimeExceptionescape the test as an error.Not changed
That this receiver is attached unconditionally, while the other two are config-gated, is left alone:
the file log is the one that has to work before the database and the config are usable, which is why
it is attached first. The defect was that it could not fail safely, not that it runs.