From a6d5dbacd9ed34336a5f155888ddb9534053ef9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Mart=C3=ADn?= Date: Wed, 5 Aug 2026 11:12:35 +0200 Subject: [PATCH] fix(api): log comp report output dir failures with exc_info (#12142) Co-authored-by: Alan Buscaglia --- .../report-output-dir-error-grouping.fixed.md | 1 + api/src/backend/api/tests/test_sentry.py | 165 +++++++++++++++++- api/src/backend/config/settings/sentry.py | 66 ++++++- api/src/backend/tasks/jobs/report.py | 11 +- api/src/backend/tasks/tests/test_reports.py | 79 +++++++++ 5 files changed, 319 insertions(+), 3 deletions(-) create mode 100644 api/changelog.d/report-output-dir-error-grouping.fixed.md diff --git a/api/changelog.d/report-output-dir-error-grouping.fixed.md b/api/changelog.d/report-output-dir-error-grouping.fixed.md new file mode 100644 index 0000000000..f2a499f910 --- /dev/null +++ b/api/changelog.d/report-output-dir-error-grouping.fixed.md @@ -0,0 +1 @@ +Compliance report output directory failures are now logged with the exception attached and fingerprinted by `errno` in Sentry, so `ENOSPC`, `ENOENT` and `EACCES` no longer share a single issue diff --git a/api/src/backend/api/tests/test_sentry.py b/api/src/backend/api/tests/test_sentry.py index 67ba1ee01e..14308f0cb6 100644 --- a/api/src/backend/api/tests/test_sentry.py +++ b/api/src/backend/api/tests/test_sentry.py @@ -1,9 +1,10 @@ +import errno import logging from unittest.mock import MagicMock, patch import pytest from config.settings import sentry as sentry_settings -from config.settings.sentry import before_send +from config.settings.sentry import before_send, errno_fingerprint def test_initialize_sentry_skips_without_dsn(): @@ -188,3 +189,165 @@ def test_before_send_passes_non_defunct_neo4j_log(): event = MagicMock() assert before_send(event, hint) == event + + +def _filesystem_hint(exception, msg="Error generating output directory"): + """Build the hint the logging integration sends for a filesystem failure.""" + exc_info = (type(exception), exception, exception.__traceback__) + log_record = _make_log_record(msg) + log_record.exc_info = exc_info + setattr( + log_record, + sentry_settings.ERROR_CATEGORY_ATTRIBUTE, + sentry_settings.FILESYSTEM_ERROR_CATEGORY, + ) + return {"log_record": log_record, "exc_info": exc_info} + + +@pytest.mark.parametrize( + ("error_number", "message", "expected_suffix"), + [ + (errno.ENOSPC, "No space left on device", "errno:ENOSPC"), + (errno.ENOENT, "No such file or directory", "errno:ENOENT"), + (errno.EACCES, "Permission denied", "errno:EACCES"), + ], +) +def test_before_send_fingerprints_oserror_by_errno( + error_number, message, expected_suffix +): + """Filesystem failures raised from the same call site must not be merged.""" + event = {} + + result = before_send(event, _filesystem_hint(OSError(error_number, message))) + + assert result is event + assert event["fingerprint"] == ["{{ default }}", expected_suffix] + + +def test_before_send_fingerprints_differ_per_errno(): + """ENOSPC and ENOENT from the same call site produce different issues.""" + enospc_event = {} + enoent_event = {} + + before_send( + enospc_event, _filesystem_hint(OSError(errno.ENOSPC, "No space left on device")) + ) + before_send( + enoent_event, + _filesystem_hint(OSError(errno.ENOENT, "No such file or directory")), + ) + + assert enospc_event["fingerprint"] != enoent_event["fingerprint"] + + +def test_before_send_fingerprints_wrapped_oserror(): + """The errno is found even when the OSError is wrapped by another error.""" + try: + try: + raise OSError(errno.ENOSPC, "No space left on device") + except OSError as os_error: + raise RuntimeError("Error generating output directory") from os_error + except RuntimeError as wrapper: + event = {} + before_send(event, _filesystem_hint(wrapper)) + + assert event["fingerprint"] == ["{{ default }}", "errno:ENOSPC"] + + +def test_before_send_does_not_fingerprint_non_oserror(): + """Non-filesystem exceptions keep Sentry's default grouping.""" + event = {} + + result = before_send(event, _filesystem_hint(ValueError("boom"))) + + assert result is event + assert "fingerprint" not in event + + +def test_before_send_does_not_fingerprint_unrelated_oserror_log(): + """Only records declaring the filesystem category opt into the errno grouping.""" + exception = OSError(errno.ENOSPC, "No space left on device") + log_record = _make_log_record("Unrelated failure") + exc_info = (OSError, exception, None) + log_record.exc_info = exc_info + event = {} + + result = before_send(event, {"log_record": log_record, "exc_info": exc_info}) + + assert result is event + assert "fingerprint" not in event + + +def test_before_send_does_not_fingerprint_exception_events(): + """Exception events without a log record keep Sentry's default grouping.""" + event = {} + + result = before_send( + event, + {"exc_info": (OSError, OSError(errno.ENOSPC, "No space left on device"), None)}, + ) + + assert result is event + assert "fingerprint" not in event + + +@pytest.mark.parametrize("fingerprint", [["scope-fingerprint"], []]) +def test_before_send_keeps_existing_fingerprint(fingerprint): + """A fingerprint set by a scope or an integration is never overwritten.""" + expected_fingerprint = fingerprint.copy() + event = {"fingerprint": fingerprint} + + before_send( + event, _filesystem_hint(OSError(errno.ENOSPC, "No space left on device")) + ) + + assert event["fingerprint"] == expected_fingerprint + + +def test_before_send_ignores_suppressed_context(): + """`raise ... from None` hides the context, so it must not group the event.""" + try: + try: + raise OSError(errno.ENOSPC, "No space left on device") + except OSError: + raise RuntimeError("Error generating output directory") from None + except RuntimeError as wrapper: + event = {} + before_send(event, _filesystem_hint(wrapper)) + + assert "fingerprint" not in event + + +def test_errno_fingerprint_follows_implicit_context(): + """An implicit `raise` during handling still exposes the original errno.""" + try: + try: + raise OSError(errno.EACCES, "Permission denied") + except OSError: + raise RuntimeError("Error generating output directory") + except RuntimeError as wrapper: + assert errno_fingerprint(wrapper) == "errno:EACCES" + + +def test_before_send_does_not_fingerprint_oserror_without_errno(): + """An OSError without errno has nothing to split the issue by.""" + event = {} + + before_send(event, _filesystem_hint(OSError("no errno here"))) + + assert "fingerprint" not in event + + +def test_errno_fingerprint_uses_raw_number_for_unknown_errno(): + """Unmapped errno values still split the issue instead of being dropped.""" + assert errno_fingerprint(OSError(9999, "unknown")) == "errno:9999" + + +def test_errno_fingerprint_stops_on_self_referencing_chain(): + """A cyclic exception chain must not hang the fingerprint lookup.""" + first = ValueError("first") + second = ValueError("second") + first.__cause__ = second + second.__cause__ = first + + assert errno_fingerprint(first) is None diff --git a/api/src/backend/config/settings/sentry.py b/api/src/backend/config/settings/sentry.py index a3acbe37bb..f5a593601e 100644 --- a/api/src/backend/config/settings/sentry.py +++ b/api/src/backend/config/settings/sentry.py @@ -1,6 +1,20 @@ +from errno import errorcode + import sentry_sdk from config.env import env +# How many links of the __cause__/__context__ chain are inspected when looking +# for the OSError that actually caused the event. +MAX_EXCEPTION_CHAIN_DEPTH = 10 + +# LogRecord attribute describing what kind of failure the record reports, set by +# the caller through `logger.exception(..., extra={"error_category": ...})`. +ERROR_CATEGORY_ATTRIBUTE = "error_category" + +# Category of records whose events are grouped by the errno of the underlying +# OSError. Only records that declare it opt into the errno fingerprint. +FILESYSTEM_ERROR_CATEGORY = "filesystem" + IGNORED_EXCEPTIONS = [ # Provider is not connected due to credentials errors "is not connected", @@ -80,6 +94,38 @@ IGNORED_EXCEPTIONS = [ ] +def errno_fingerprint(exception): + """ + Return an errno-based fingerprint suffix for OSError-like exceptions. + + Filesystem failures such as ENOSPC (disk full), ENOENT (missing mount point) + or EACCES (wrong permissions) are all OSError raised from the same call + site, so Sentry's default grouping merges them into a single issue even + when the exception is attached to the event. Appending the errno keeps the + default grouping and splits the issue per failure cause. + + Only the part of the chain Sentry itself displays is inspected: a + `raise ... from None` sets __suppress_context__, so the implicit + __context__ is dropped from the event and must not group it either. + + Returns None when no OSError with an errno is found in the exception chain. + """ + seen = set() + for _ in range(MAX_EXCEPTION_CHAIN_DEPTH): + if exception is None or id(exception) in seen: + break + seen.add(id(exception)) + if isinstance(exception, OSError) and exception.errno is not None: + return f"errno:{errorcode.get(exception.errno, exception.errno)}" + if exception.__cause__ is not None: + exception = exception.__cause__ + elif exception.__suppress_context__: + break + else: + exception = exception.__context__ + return None + + def before_send(event, hint): """ before_send handles the Sentry events in order to send them or not @@ -115,10 +161,28 @@ def before_send(event, hint): # Ignore exceptions with the ignored_exceptions if "exc_info" in hint and hint["exc_info"]: - exc_value = str(hint["exc_info"][1]) + exception = hint["exc_info"][1] + exc_value = str(exception) if any(ignored in exc_value for ignored in IGNORED_EXCEPTIONS): return None # Explicitly return None to drop the event + # Split filesystem issues per errno instead of grouping every failure raised + # from the same call site under a single issue. Only records that declare + # themselves as filesystem failures opt in, and a fingerprint already set by + # a scope or an integration always wins. + log_record = hint.get("log_record") + exc_info = hint.get("exc_info") + if ( + log_record is not None + and exc_info + and getattr(log_record, ERROR_CATEGORY_ATTRIBUTE, None) + == FILESYSTEM_ERROR_CATEGORY + and "fingerprint" not in event + ): + fingerprint_suffix = errno_fingerprint(exc_info[1]) + if fingerprint_suffix: + event["fingerprint"] = ["{{ default }}", fingerprint_suffix] + return event diff --git a/api/src/backend/tasks/jobs/report.py b/api/src/backend/tasks/jobs/report.py index c9d63a63ac..6750c58f77 100644 --- a/api/src/backend/tasks/jobs/report.py +++ b/api/src/backend/tasks/jobs/report.py @@ -13,6 +13,7 @@ from api.db_utils import rls_transaction from api.models import Provider, Scan, ScanSummary, StateChoices, ThreatScoreSnapshot from celery.utils.log import get_task_logger from config.django.base import DJANGO_TMP_OUTPUT_DIRECTORY +from config.settings.sentry import ERROR_CATEGORY_ATTRIBUTE, FILESYSTEM_ERROR_CATEGORY from prowler.lib.check.compliance_models import ( Compliance, get_bulk_compliance_frameworks_universal, @@ -960,7 +961,15 @@ def generate_compliance_reports( first_output_path = next(iter(output_paths.values())) out_dir = str(Path(first_output_path).parent.parent) except Exception as e: - logger.error("Error generating output directory: %s", e) + # logger.exception attaches the exception (and its traceback) to the + # Sentry event and the filesystem category opts that event into the + # errno fingerprint, so ENOSPC, ENOENT and EACCES raised from this same + # call site land on separate issues. + logger.exception( + "Error generating output directory: %s", + e, + extra={ERROR_CATEGORY_ATTRIBUTE: FILESYSTEM_ERROR_CATEGORY}, + ) error_dict = {"error": str(e), "upload": False, "path": ""} if generate_threatscore: results["threatscore"] = error_dict.copy() diff --git a/api/src/backend/tasks/tests/test_reports.py b/api/src/backend/tasks/tests/test_reports.py index 626237922f..ac6b288aa3 100644 --- a/api/src/backend/tasks/tests/test_reports.py +++ b/api/src/backend/tasks/tests/test_reports.py @@ -1,3 +1,5 @@ +import errno +import logging import os import time import uuid @@ -14,6 +16,11 @@ from api.models import ( StateChoices, StatusChoices, ) +from config.settings.sentry import ( + ERROR_CATEGORY_ATTRIBUTE, + FILESYSTEM_ERROR_CATEGORY, + before_send, +) from prowler.lib.check.models import Severity from reportlab.lib import colors from tasks.jobs.report import ( @@ -1676,6 +1683,78 @@ class TestGenerateComplianceReportsCIS: assert result["cis"]["upload"] is False assert result["cis"]["error"] == "dir boom" + @patch("tasks.jobs.report._aggregate_requirement_statistics_from_database") + @patch("tasks.jobs.report._generate_compliance_output_directory") + @patch("tasks.jobs.report.Compliance.get_bulk") + def test_output_directory_failures_are_grouped_per_errno( + self, + mock_get_bulk, + mock_generate_output_dir, + mock_stats, + monkeypatch, + caplog, + tenants_fixture, + scans_fixture, + aws_provider, + ): + """A full disk and a missing mount point must not share a Sentry issue. + + Both are OSError raised from the same ``os.makedirs`` call, so they only + stay apart if the exception reaches ``before_send``, which fingerprints + it by errno. + """ + tenant = tenants_fixture[0] + scan = scans_fixture[0] + provider = aws_provider + + self._force_scan_has_findings(monkeypatch) + mock_stats.return_value = {} + mock_get_bulk.return_value = {"cis_5.0_aws": Mock()} + + fingerprints = [] + for error_number, message in ( + (errno.ENOSPC, "No space left on device: '/tmp/prowler_api_output'"), + (errno.ENOENT, "No such file or directory: '/mnt/output'"), + ): + mock_generate_output_dir.side_effect = OSError(error_number, message) + caplog.clear() + + with caplog.at_level(logging.ERROR, logger="tasks.jobs.report"): + generate_compliance_reports( + tenant_id=str(tenant.id), + scan_id=str(scan.id), + provider_id=str(provider.id), + generate_threatscore=False, + generate_ens=False, + generate_nis2=False, + generate_csa=False, + generate_cis=True, + ) + + record = next( + record + for record in caplog.records + if "Error generating output directory" in record.getMessage() + ) + # Without exc_info the Sentry event carries no exception at all and + # nothing can tell the two failures apart. + assert record.exc_info is not None + # The category is what scopes the errno fingerprint to this record. + assert ( + getattr(record, ERROR_CATEGORY_ATTRIBUTE, None) + == FILESYSTEM_ERROR_CATEGORY + ) + + # Same hint the Sentry logging integration builds for this record. + event = {} + before_send(event, {"log_record": record, "exc_info": record.exc_info}) + fingerprints.append(event["fingerprint"]) + + assert fingerprints == [ + ["{{ default }}", "errno:ENOSPC"], + ["{{ default }}", "errno:ENOENT"], + ] + class TestPickLatestCisVariant: """Unit tests for `_pick_latest_cis_variant` helper."""