Skip to content

fix(topology): scope alerts to the owning application before creating/updating incidents - #6734

Open
Prabal864 wants to merge 1 commit into
keephq:mainfrom
Prabal864:fix-6701-topology-cross-application-leak
Open

fix(topology): scope alerts to the owning application before creating/updating incidents#6734
Prabal864 wants to merge 1 commit into
keephq:mainfrom
Prabal864:fix-6701-topology-cross-application-leak

Conversation

@Prabal864

Copy link
Copy Markdown
Contributor

What

Fixes #6701.

TopologyProcessor._process_tenant builds one tenant-wide services_to_alerts dict (every alert for every service in the tenant), then passes that same, unscoped dict into _create_application_based_incident / _update_application_based_incident for every application in the loop:

self._update_application_based_incident(
    tenant_id, application, incident, services_to_alerts
)
...
self._create_application_based_incident(
    tenant_id, application, services_to_alerts
)

Both of those methods just iterate every key in whatever dict they're handed (see the loop building alerts in _update_application_based_incident), so an application's incident ends up including alerts from every other application's services too - not just its own.

The method already computes services_with_alerts, the list of this application's own services that have alerts, a few lines above - it's just never used to scope what gets passed on.

Fix

Build application_services_to_alerts, restricted to services_with_alerts, and pass that into both incident methods instead of the raw tenant-wide dict.

Testing

Added tests/test_topology_processor.py, mocking every DB-touching collaborator (_get_topology_data, _get_applications_data, _get_application_based_incident, get_last_alerts, convert_db_alerts_to_dto_alerts) so the scoping logic in _process_tenant can be exercised without a live database. Covers both the create-incident and update-incident paths, asserting each application's incident only ever sees its own service's alerts.

Same environment limitation as my previous PRs here: I couldn't run the suite on this Windows sandbox - tests/conftest.py imports pytest_docker and requires live Docker/MySQL fixtures at collection time, regardless of which test is targeted. What I did verify:

  • python -m py_compile passes on both changed files.
  • black --check and isort --check report no new issues on the changed files.
  • Extracted the exact before/after scoping logic into a standalone script (plain dicts/lists standing in for the DB models) and confirmed the old logic leaks a second application's alerts into the first application's incident, while the new logic does not.

A maintainer running this on Linux/macOS should see tests/test_topology_processor.py pass outright.

Checklist

  • I have read the Contributing Guide
  • If you've added code that should be tested, add tests.
  • If you've changed APIs, update the documentation. — not applicable, no API change.
  • Ensure the test suite passes.
  • Make sure your code lints (black/isort) — for the lines this PR actually touches.

…/updating incidents

Closes keephq#6701.

_process_tenant() builds one tenant-wide services_to_alerts dict, then passed
that same dict unscoped into _create_application_based_incident and
_update_application_based_incident for every application. Both methods just
iterate every key in whatever dict they're given (see the loop in
_update_application_based_incident), so an application's incident ends up
containing alerts from every other application's services too, not just its
own.

Fix: build application_services_to_alerts, scoped to the current
application's own services (services_with_alerts, which was already being
computed correctly a few lines above but never used for this), and pass that
scoped dict into both incident methods instead of the raw tenant-wide one.

Testing:
- Added tests/test_topology_processor.py, mocking every DB-touching
  collaborator (_get_topology_data, _get_applications_data,
  _get_application_based_incident, get_last_alerts,
  convert_db_alerts_to_dto_alerts) so the scoping logic in _process_tenant
  can be exercised without a live database. Covers both the create-incident
  and update-incident paths.
- Could not run the suite here: tests/conftest.py imports pytest_docker and
  requires live Docker/MySQL fixtures at collection time regardless of which
  test is targeted, on this Windows sandbox.
- Verified instead: python -m py_compile on both changed files; black --check
  and isort --check report no new issues; extracted the exact before/after
  scoping logic into a standalone script (using plain dicts/lists standing in
  for the DB models) and confirmed the old logic leaks a second
  application's alerts into the first application's incident while the new
  logic does not.
@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. Bug Something isn't working labels Aug 23, 2026
@codecov

codecov Bot commented Aug 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 46.41%. Comparing base (ce302f6) to head (a354bbc).
⚠️ Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
keep/topologies/topology_processor.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6734      +/-   ##
==========================================
- Coverage   46.42%   46.41%   -0.01%     
==========================================
  Files         178      178              
  Lines       18694    18698       +4     
==========================================
  Hits         8679     8679              
- Misses      10015    10019       +4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Aug 23, 2026
Comment thread keep/entrypoint.sh Outdated
Comment on lines +29 to +35
# Run migrations synchronously first so the database schema is fully initialized
# before ARQ workers and API gunicorn start querying tables.
python -c "from keep.api.core.db_on_start import migrate_db; migrate_db()" || {
echo "Failed to run migrations before starting workers, exiting"
exit 1
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this has many implications, not sure we want to do that as part of this fix, why its mandatory?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, that's not related to this fix, removing it.

@Prabal864
Prabal864 force-pushed the fix-6701-topology-cross-application-leak branch from e6f412b to a354bbc Compare August 24, 2026 05:54
@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Aug 24, 2026
@Prabal864
Prabal864 requested a review from shahargl August 27, 2026 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: Topology Processor assigns alerts from unrelated applications to each application incident

2 participants