Skip to content

fix(keep-provider): don't mutate the shared fingerprint_fields list - #6720

Merged
shahargl merged 1 commit into
keephq:mainfrom
Prabal864:fix/keep-provider-fingerprint-mutation
Aug 22, 2026
Merged

fix(keep-provider): don't mutate the shared fingerprint_fields list#6720
shahargl merged 1 commit into
keephq:mainfrom
Prabal864:fix/keep-provider-fingerprint-mutation

Conversation

@Prabal864

@Prabal864 Prabal864 commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

What

Fixes #6719.

KeepProvider._build_alert did fingerprint_fields.append("workflowId") on whatever list was passed in. _notify_alert's per-alert loop passes that same list object into _build_alert on every iteration of a foreach batch, so each alert after the first accumulated one more duplicate "workflowId" entry, which changes its computed SHA-256 fingerprint.

Fix

  • keep_provider.py:177 - build a new list (fingerprint_fields + ["workflowId"]) instead of mutating the caller's list with .append.
  • keep_provider.py:139 - switched the mutable default argument (fingerprint_fields=[]) to None while touching this line, since if not fingerprint_fields: already treats None/[] identically, so this is a no-op behaviorally but removes the classic mutable-default footgun flagged alongside the main bug.

Testing

Added tests/test_keep_provider_fingerprint_fields_mutation.py:

  • test_build_alert_does_not_mutate_caller_fingerprint_fields - calls _build_alert 3x with the same fingerprint_fields list (mirroring _notify_alert's loop) and asserts the list is unchanged afterward.
  • test_build_alert_produces_consistent_fingerprints_across_a_batch - asserts an alert's fingerprint is identical whether it's built standalone or as the 3rd alert in a batch that reused the same fingerprint_fields object.

I wasn't able to run the full suite in my sandbox - pip install -e . fails on this platform because uvloop doesn't support Windows (a pyproject dependency), and tests/conftest.py needs Docker/MySQL fixtures at collection time regardless of which test is targeted. I verified both the bug and the fix by extracting the exact control flow (_build_alert's fingerprint-fields handling and the real BaseProvider.get_alert_fingerprint hashing algorithm) into a standalone script and running it directly - before the fix, a 3-alert batch left fingerprint_fields as ['labels.service', 'workflowId', 'workflowId', 'workflowId'] and alert 3's fingerprint didn't match the correct value; after the fix, the list stays ['labels.service'] and the fingerprint matches. python -m py_compile and black --check / isort --check pass on both changed files.

@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. Bug Something isn't working labels Aug 21, 2026
@CLAassistant

CLAassistant commented Aug 21, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

_build_alert called fingerprint_fields.append("workflowId") on whatever
list was passed in. _notify_alert's per-alert loop passes the same list
object into _build_alert on every iteration of a foreach batch, so each
alert after the first accumulated one more duplicate "workflowId" entry,
changing its computed fingerprint.

Build a new list instead of appending in place, and switch the mutable
default argument to None while at it.

Fixes keephq#6719
@Prabal864
Prabal864 force-pushed the fix/keep-provider-fingerprint-mutation branch from fabc6ec to 1f9e634 Compare August 21, 2026 20:57

@shahargl shahargl 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.

Lgtm

@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Aug 22, 2026
@shahargl
shahargl merged commit 174af84 into keephq:main Aug 22, 2026
5 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

🔥 Fantastic work @Prabal864! Your very first PR to keep has been merged! 🎉🥳

You've just taken your first step into open-source, and we couldn't be happier to have you onboard. 🙌
If you're feeling adventurous, why not dive into another issue and keep contributing? The community would love to see more from you! 🚀

For any support, feel free to reach out on the community: https://slack.keephq.dev. Happy coding! 👩‍💻👨‍💻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working lgtm This PR has been approved by a maintainer 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]: KeepProvider mutates the shared fingerprint_fields list across alerts in a foreach batch, corrupting fingerprints

3 participants