Skip to content

fix: keep alert id stable when labeling a solo sequence FP - #584

Merged
fe51 merged 4 commits into
mainfrom
fix/label-single-sequence-alert-churn
May 15, 2026
Merged

fe51 merged 4 commits into
mainfrom
fix/label-single-sequence-alert-churn

Conversation

@MateoLostanlen

Copy link
Copy Markdown
Member
  • When a sequence labeled non-wildfire is the only one in its alert, the previous flow detached it, deleted the (now empty) alert, and created an identical replacement — churning the alert id for no reason.
  • Now the solo case is detected via a LIMIT 1 siblings probe and the alert is left untouched.
  • Multi-sequence alerts still split out the FP into its own alert so it stops contributing to triangulation.
  • Adds an integration test for the stable-id case and a unit test for the early-return branch; updates the existing multi-sequence unit test to mock the new probe.

When a sequence labeled non-wildfire is the only one in its alert,
the previous logic detached it, deleted the alert (now empty), and
created an identical replacement — changing the alert id for no
gain. Multi-sequence alerts still split so the FP no longer
contributes to triangulation.
Adds an integration test asserting the alert id is preserved when
labeling a single-sequence alert as non-wildfire, and a unit test
for the same path. Updates the existing multi-sequence unit test
to mock the new siblings probe.
@codecov

codecov Bot commented May 5, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.11%. Comparing base (b4fa415) to head (c6e93b9).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #584      +/-   ##
==========================================
+ Coverage   89.80%   90.11%   +0.30%     
==========================================
  Files          55       55              
  Lines        2422     2508      +86     
==========================================
+ Hits         2175     2260      +85     
- Misses        247      248       +1     
Flag Coverage Δ
backend ?
client ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 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.

@MateoLostanlen
MateoLostanlen marked this pull request as draft May 5, 2026 17:33
@MateoLostanlen
MateoLostanlen marked this pull request as ready for review May 14, 2026 07:16
@MateoLostanlen
MateoLostanlen requested a review from fe51 May 14, 2026 07:16

@fe51 fe51 left a comment

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.

Thanks a lot for the PR @MateoLostanlen ! tested and work on devenv

Just a comment and suggestion, not blocking but wondering of would more relevant. Actually, current isolation is probably fine but worth tightening since the fix is trivial. Tell me what do you think

Comment thread src/tests/endpoints/test_sequences.py Outdated
Comment on lines +495 to +497
alerts_res = await detection_session.exec(select(Alert).execution_options(populate_existing=True))
alerts_rows = alerts_res.all()
assert [row.id for row in alerts_rows] == [original_alert_id]

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.

The two assertions query the entire Alert and AlertSequence tables, so they implicitly assume no other rows exist in the DB at that point. If test isolation ever weakens (shared fixtures, parallel runs, leftover data), these will fail for the wrong reason.

Scoping both queries to the sequence under test makes them resilient to whatever else is in the DB:

Fetches only AlertSequence rows for THIS sequence, checks it still points to the original alert

Suggested change
alerts_res = await detection_session.exec(select(Alert).execution_options(populate_existing=True))
alerts_rows = alerts_res.all()
assert [row.id for row in alerts_rows] == [original_alert_id]
alert_links_res = await detection_session.exec(
select(AlertSequence.alert_id).where(AlertSequence.sequence_id == seq.id)...
)
assert set(alert_links_res.all()) == {original_alert_id}

Comment thread src/tests/endpoints/test_sequences.py Outdated
@MateoLostanlen
MateoLostanlen requested a review from fe51 May 15, 2026 11:37
@MateoLostanlen

Copy link
Copy Markdown
Member Author

Thanks @fe51 I added your change

@fe51 fe51 left a comment

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.

All good to merge now !

@fe51
fe51 merged commit bf42eed into main May 15, 2026
23 checks passed
@fe51
fe51 deleted the fix/label-single-sequence-alert-churn branch May 15, 2026 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants