starred/netbox
1
0
Fork 0
mirror of https://github.com/netbox-community/netbox.git synced 2026-01-11 21:10:29 +01:00
Code Issues 273 Packages Projects Releases 10 Wiki Activity

Per‑sender duplicate signal registration causes CounterCacheField double‑counting #11787

New Issue
Closed
opened 2025-12-29 21:49:52 +01:00 by adam · 2 comments
adam commented 2025-12-29 21:49:52 +01:00
Owner
Copy Link

Originally created by @pheus on GitHub (Oct 28, 2025).

Originally assigned to: @pheus on GitHub.

NetBox Edition

NetBox Community

NetBox Version

v4.4.4

Python Version

3.10

Steps to Reproduce

  1. Have two CounterCacheField registrations that both use dcim.Device as the child model, for example:
    • DeviceType.device_count (count devices by device_type)
    • VirtualChassis.member_count (count devices by virtual_chassis)
  2. Create or delete a single Device.

Expected Behavior

Each create/delete updates the relevant counters once (e.g., device_count += 1 on create, device_count -= 1 on delete).

Observed Behavior

Counters update twice (e.g., +2 on create, -2 on delete).

Root Cause

In the counters registry (utilities/counters.py), the dispatch_uid for signal connections is currently formed from the model and field (conceptually {model._meta.label}.{field.name}).
When multiple counters share the same sender (e.g., Device via device_type_id and virtual_chassis_id), this yields different UIDs, so Django connects the same receiver multiple times for that sender. A single post_save emission for Device is then handled twice, doubling the counter updates.

Related Work

This was uncovered while developing FR #19523 (instance/usage filtering), which depends on accurate device_count/module_count values.

Originally created by @pheus on GitHub (Oct 28, 2025). Originally assigned to: @pheus on GitHub. ### NetBox Edition NetBox Community ### NetBox Version v4.4.4 ### Python Version 3.10 ### Steps to Reproduce 1. Have two `CounterCacheField` registrations that both use **`dcim.Device`** as the child model, for example: - `DeviceType.device_count` (count devices by `device_type`) - `VirtualChassis.member_count` (count devices by `virtual_chassis`) 2. Create or delete a single `Device`. ### Expected Behavior Each create/delete updates the relevant counters **once** (e.g., `device_count += 1` on create, `device_count -= 1` on delete). ### Observed Behavior Counters update **twice** (e.g., `+2` on create, `-2` on delete). ### Root Cause In the counters registry (`utilities/counters.py`), the `dispatch_uid` for signal connections is currently formed from the **model and field** (conceptually `{model._meta.label}.{field.name}`). When multiple counters share the **same sender** (e.g., `Device` via `device_type_id` and `virtual_chassis_id`), this yields **different UIDs**, so Django connects the **same** receiver **multiple times** for that sender. A single `post_save` emission for `Device` is then handled twice, doubling the counter updates. ### Related Work This was uncovered while developing FR **#19523** (instance/usage filtering), which depends on accurate `device_count`/`module_count` values.
adam added the type: bugstatus: acceptednetboxseverity: low labels 2025-12-29 21:49:52 +01:00
adam closed this issue 2025-12-29 21:49:52 +01:00
adam commented 2025-12-29 21:49:53 +01:00
Author
Owner
Copy Link

@pheus commented on GitHub (Oct 28, 2025):

Proposed Fix

Make signal connections idempotent per sender, not per field. This ensures there’s exactly one receiver per child model while still updating all registered counters for that sender.

Updated connect_counters():

from django.apps import apps
from django.db.models.signals import post_save, pre_delete, post_delete

def connect_counters(*models):
    """
    Register counter fields and connect signal handlers for their child models.
    Ensures exactly one receiver per child (sender), even when multiple counters
    reference the same sender (e.g., Device).
    """
    connected = set()  # child models we've already connected

    for model in models:

        # Find all CounterCacheFields on the model
        counter_fields = [
            field for field in model._meta.get_fields() if isinstance(field, CounterCacheField)
        ]

        for field in counter_fields:
            to_model = apps.get_model(field.to_model_name)

            # Register the counter in the registry
            change_tracking_fields = registry['counter_fields'][to_model]
            change_tracking_fields[f"{field.to_field_name}_id"] = field.name

            # Connect signals once per child model
            if to_model in connected:
                continue

            # Ensure dispatch_uid is unique per model (sender), not per field
            uid_base = f"countercache.{to_model._meta.label_lower}"

            # Connect the post_save and post_delete handlers
            post_save.connect(
                post_save_receiver,
                sender=to_model,
                weak=False,
                dispatch_uid=f"{uid_base}.post_save",
            )
            pre_delete.connect(
                pre_delete_receiver,
                sender=to_model,
                weak=False,
                dispatch_uid=f"{uid_base}.pre_delete",
            )
            post_delete.connect(
                post_delete_receiver,
                sender=to_model,
                weak=False,
                dispatch_uid=f"{uid_base}.post_delete",
            )

            connected.add(to_model)

This keeps a single receiver per sender (Device) while allowing that receiver to process all registered counters (e.g., device_count and member_count) in one pass.

I’m happy to open a PR targeting main if this approach looks good.

@pheus commented on GitHub (Oct 28, 2025): ### Proposed Fix Make signal connections **idempotent per sender**, not per field. This ensures there’s exactly **one** receiver per child model while still updating all registered counters for that sender. **Updated `connect_counters()`:** ```python from django.apps import apps from django.db.models.signals import post_save, pre_delete, post_delete def connect_counters(*models): """ Register counter fields and connect signal handlers for their child models. Ensures exactly one receiver per child (sender), even when multiple counters reference the same sender (e.g., Device). """ connected = set() # child models we've already connected for model in models: # Find all CounterCacheFields on the model counter_fields = [ field for field in model._meta.get_fields() if isinstance(field, CounterCacheField) ] for field in counter_fields: to_model = apps.get_model(field.to_model_name) # Register the counter in the registry change_tracking_fields = registry['counter_fields'][to_model] change_tracking_fields[f"{field.to_field_name}_id"] = field.name # Connect signals once per child model if to_model in connected: continue # Ensure dispatch_uid is unique per model (sender), not per field uid_base = f"countercache.{to_model._meta.label_lower}" # Connect the post_save and post_delete handlers post_save.connect( post_save_receiver, sender=to_model, weak=False, dispatch_uid=f"{uid_base}.post_save", ) pre_delete.connect( pre_delete_receiver, sender=to_model, weak=False, dispatch_uid=f"{uid_base}.pre_delete", ) post_delete.connect( post_delete_receiver, sender=to_model, weak=False, dispatch_uid=f"{uid_base}.post_delete", ) connected.add(to_model) ``` This keeps a single receiver per sender (`Device`) while allowing that receiver to process all registered counters (e.g., `device_count` and `member_count`) in one pass. --- I’m happy to open a PR targeting `main` if this approach looks good.
adam commented 2025-12-29 21:49:53 +01:00
Author
Owner
Copy Link

@jnovinger commented on GitHub (Oct 28, 2025):

Thanks, @pheus. Assigned to you.

@jnovinger commented on GitHub (Oct 28, 2025): Thanks, @pheus. Assigned to you.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
beta
breaking change
complexity: high
complexity: low
complexity: medium
needs milestone
netbox
pending closure
plugin candidate
pull-request
Mirrored from GitHub Pull Request
 severity: high
severity: low
severity: medium
status: accepted
status: backlog
status: blocked
status: duplicate
status: needs owner
status: needs triage
status: revisions needed
status: under review
topic: GraphQL
topic: Internationalization
topic: OpenAPI
topic: UI/UX
topic: cabling
topic: event rules
topic: htmx navigation
topic: industrialization
topic: migrations
topic: plugins
topic: scripts
topic: templating
topic: testing
type: bug
type: deprecation
type: documentation
type: feature
type: housekeeping
type: translation
No Label netbox severity: low status: accepted type: bug
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
adam
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/netbox#11787
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?