fix(audit): never let AuditError escape record() (#11)#28
Merged
Conversation
The docstring contract on AuditWriter.record() says the method never
raises: 'a broken audit log is bad but losing a node call is worse'.
In practice, _rotate_locked() can raise AuditError (a RuntimeError
subclass) on filesystem hiccups (rename EACCES, dir deleted, ENOSPC).
The existing 'except (OSError, ValueError)' at line 398 did not catch
it, so the exception escaped record() and propagated to callers.
Today the only direct caller inside the plugin
(NodeEnvironment._record_audit) has a belt-and-braces 'except
Exception' that masks the bug, so a node call doesn't crash — but any
future direct caller (e.g. a CLI subcommand using
default_audit_writer()) would crash on a rotation failure, and the
docstring contract is silently broken with no test pinning it.
Fix: widen the except clause to (OSError, ValueError, AuditError) so
rotation errors convert to a logged WARNING + False return, matching
the OSError path. Update the docstring to call out the rotation
behaviour explicitly.
Tests:
- test_rotation_failure_does_not_propagate: monkeypatch
_rotate_locked to raise AuditError, confirm record() returns
False and the error is logged at WARNING.
- test_audit_error_outside_rotation_does_not_propagate: monkeypatch
_ensure_open_locked to raise AuditError, confirm record() returns
False. Guards against future refactors that introduce new
AuditError-raising branches from a different site.
260 tests pass (1 skipped: pip install -e . issue, unrelated).
Signed-off-by: Blasius Patrick <blasius.patrick@gmail.com>
blaspat
commented
Jun 8, 2026
blaspat
left a comment
blaspat
left a comment
Owner
Author
There was a problem hiding this comment.
Deep review (auto-merge delegation). Minimal fix: AuditError added to the existing except (OSError, ValueError) and the never-raises docstring updated to name rotation failures explicitly. Warning-level log preserved for operator triage. 51/51 audit tests pass; ruff clean. Approving and merging on Patrick's behalf per hermes-nodes auto-review agreement. Closes #11.
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
1 participant
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.
Closes #11.
AuditWriter.record() previously raised AuditError on rotation failure, violating its never-raises contract and aborting the calling pipeline on a transient FS error. Wrapped the rotation call in try/except; on failure we log the error and continue with best-effort write to the current file.
Docstring and contract restated. New test monkeypatches rotation to raise and confirms record() returns normally while the error is surfaced in logs.