fix(logfiles): harden ignore rules parsing and validation - #3578
Conversation
A malformed line in an ignore rules file crashed the whole log check with a ValueError. The crash happened in the cluster manager fixture teardown, so the cleanup was skipped and every subsequent log check on the cluster instance kept crashing until the rules file was manually removed. Skip malformed lines with a warning instead - skipping a rule can only lead to reporting previously ignored errors. Validate rules when they are added, so that a hazardous rule fails the offending test loudly at the source: * Reject ";;" in the files glob and line breaks in the glob or the regex - they would corrupt the line-based rules file format. * Reject a regex that matches an empty string - as an empty branch of the combined alternation it would suppress all errors. * Reject a regex that doesn't compile (as str and as bytes - the log search compiles the combined regex as bytes) or that cannot be combined with other rules (e.g. global inline flags) - it would crash every log check on compile. * Reject a non-finite or negative expire time - it would silently turn "expire after X" into "never expire". Apply the same checks with warning and skip when the combined ignore regex is built, so that hand-edited rules files and conflicts between rules (e.g. a redefined group name) cannot crash the log checks either. The rules are combined in sorted order, so the outcome is deterministic when rules conflict. Warnings are logged once per process, as the rules files are parsed again in every log search.
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Hardens ignore-rules handling in utils.logfiles to prevent malformed or hazardous ignore rules from crashing log checks, and adds targeted validation and tests for these cases.
Changes:
- Add warn-once logging and make ignore-rules parsing resilient to malformed lines.
- Validate ignore rules at creation time (
add_ignore_rule) and skip hazardous/invalid regexes when building the combined ignore regex. - Extend test coverage for malformed lines, invalid rules, and hazardous regex combinations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| framework_tests/test_logfiles.py | Adds fixtures and parametrized tests verifying malformed rules are warned+skipped and hazardous rules are rejected or skipped safely. |
| cardano_node_tests/utils/logfiles.py | Implements warn-once mechanism, resilient parsing, regex validation/combination checks, and stronger add_ignore_rule validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* Reject non-finite and negative expire times also when parsing the rules files, as hand-edited values like "nan" would silently turn the expire time into "never expire". * Bound the warn-once deduplication memory with an LRU cache. * Avoid rebuilding the valid regexes list on every iteration when building the combined ignore regex - append and pop on failure. * Use explicit utf-8 encoding for consistency.
Use explicit utf-8 encoding also in the write-side regex check, fix the warn-once docstring for the bounded LRU cache semantics, and pin the warning deduplication with a test.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
framework_tests/test_logfiles.py:18
- The new autouse fixture is missing a return type annotation, which makes it inconsistent with the rest of this module’s typed fixtures and reduces type-checker usefulness.
@pytest.fixture(autouse=True)
def _reset_logged_warnings():
"""Reset the warn-once dedup cache, so tests don't depend on the execution order."""
logfiles._warn_once.cache_clear()
A malformed line in an ignore rules file crashed the whole log check with a ValueError. The crash happened in the cluster manager fixture teardown, so the cleanup was skipped and every subsequent log check on the cluster instance kept crashing until the rules file was manually removed. Skip malformed lines with a warning instead - skipping a rule can only lead to reporting previously ignored errors.
Validate rules when they are added, so that a hazardous rule fails the offending test loudly at the source:
Apply the same checks with warning and skip when the combined ignore regex is built, so that hand-edited rules files and conflicts between rules (e.g. a redefined group name) cannot crash the log checks either. The rules are combined in sorted order, so the outcome is deterministic when rules conflict. Warnings are logged once per process, as the rules files are parsed again in every log search.