Skip to content

Fixes #21498: Fix Exception when changing a Cable Termination with an Interface Event Rule#21815

Merged
arthanson merged 1 commit intomainfrom
21498-eventrules-in-v452-and-v453-result-in-server-error
Apr 2, 2026
Merged

Fixes #21498: Fix Exception when changing a Cable Termination with an Interface Event Rule#21815
arthanson merged 1 commit intomainfrom
21498-eventrules-in-v452-and-v453-result-in-server-error

Conversation

@pheus
Copy link
Copy Markdown
Contributor

@pheus pheus commented Apr 1, 2026

Fixes: #21498

This PR fixes a regression in deferred event serialization during cable edits.

When a cable termination is changed, CablePath rows can be deleted and recreated while endpoint instances are still in memory. If an event rule is active, the deferred serialization introduced in v4.5.2 can then hit a stale _path reference and raise CablePath.DoesNotExist.

The DCIM-side fix makes PathEndpoint.path recover from a stale _path reference with a narrow refresh of that field, and routes the remaining internal callers through path instead of accessing _path directly. This prevents the crash and keeps the fix scoped to the denormalized relation involved in path rebuilds.

This PR also includes the EventContext changes intentionally. While the PathEndpoint fix prevents the exception, it does not by itself address stale event payloads when the same object is enqueued multiple times within one request. EventContext now tracks the latest serialization source for coalesced duplicate enqueues, so deferred serialization uses the most recent queued instance while keeping the public object entry unchanged. Delete events are still serialized eagerly before the row becomes unavailable.

Finally, process_event_rules() now avoids mutating event_rule.action_data in place when merging event payload data.

Together, these changes fix the traceback from #21498, preserve the deferred-serialization optimization, and ensure event consumers receive the final queued state for coalesced updates.

@pheus
Copy link
Copy Markdown
Contributor Author

pheus commented Apr 1, 2026

@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration.

@claude
Copy link
Copy Markdown

claude bot commented Apr 1, 2026

Claude finished @pheus's task in 2m 17s —— View job


Code Review

  • Read changed files
  • Analyze for issues of concern
  • Post review

Issues of Concern

1. @skipIf decorator silently disables test_send_webhook (high concern)

netbox/extras/tests/test_event_rules.py:345 — A @skipIf('netbox.tests.dummy_plugin' not in settings.PLUGINS, ...) decorator was added to test_send_webhook, which previously ran unconditionally. This will silently skip an existing regression test in any environment (including CI) where dummy_plugin is not configured in PLUGINS.

There's no explanation in the PR for why this skip condition was introduced. If the test was legitimately failing due to a missing dependency, that should be addressed directly. As written, this disables a test that was working before and provides no coverage for the webhook dispatch path in standard environments. Fix this →

2. Potential unhandled ObjectDoesNotExist in PathEndpoint.path after refresh

netbox/dcim/models/device_components.py:360-372 — The recovery path in path is:

except ObjectDoesNotExist:
    self.refresh_from_db(fields=['_path'])
    return self._path if self._path_id else None

After refresh_from_db, if self._path_id is still non-None (e.g., the FK was updated to a new CablePath that is itself already gone), accessing self._path in the return will raise ObjectDoesNotExist again — this time unhandled. The PR comment says "not persistent database corruption," which is a reasonable assumption, but the unhandled second raise is worth acknowledging or guarding against explicitly.

3. get_connected_endpoints_reachable docstring overhead (minor)

netbox/dcim/api/serializers_/base.py:41-50 — A multi-line docstring was added to a small @extend_schema_field method. The content is generic and adds noise without meaningful information. Not a functional concern, but inconsistent with the existing codebase style for small helpers.


Overall the core logic — routing through path instead of _path directly, the EventContext lazy/eager serialization split, and the action_data copy-before-merge fix — looks correct and well-targeted. The @skipIf issue is the main thing that needs explanation or reverting before merge.

@pheus
Copy link
Copy Markdown
Contributor Author

pheus commented Apr 1, 2026

Thanks for the review.

1. @skipIf on test_send_webhook: Intentional, test_send_webhook already depends on the dummy plugin implicitly; the decorator makes that explicit. Happy to defer to maintainers on whether to keep it in this PR.

2. Unhandled ObjectDoesNotExist after refresh: The described scenario would require two consecutive path deletions targeting the same endpoint between refresh_from_db and the FK access. The actual rebuild sequence (delete -> create -> update FK) means the refreshed _path_id is either valid or NULL. A second try/except would obscure intent without guarding against a realistic failure.

3. Docstring: Fair nit. The inline comment explaining why path is used instead of _path is the valuable part. Leaving as-is for maintainer preference.

@pheus pheus requested review from a team and arthanson and removed request for a team April 1, 2026 14:54
Copy link
Copy Markdown
Collaborator

@arthanson arthanson left a comment

Choose a reason for hiding this comment

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

great work @pheus just some minor comments, - can we not reformat the comments for just line length changes unless there is a specific reason? quite a few of the changes are just reformatting comments without any wording changes.

deleted, refresh only the `_path` field from the database and retry.
This keeps the fix cheap and narrowly scoped to the denormalized FK.
"""
if self._path_id is None:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think this check is needed, the try below should return None if _path_id is None

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.

For PathEndpoint.path, I’d like to keep the early _path_id is None branch as a small fast-path/readability check, but I agree the important part is the second guarded lookup after refresh_from_db().

@pheus pheus force-pushed the 21498-eventrules-in-v452-and-v453-result-in-server-error branch 2 times, most recently from 5384ef2 to a398601 Compare April 2, 2026 22:01
Cable edits can delete and recreate CablePath rows while endpoint
instances remain in memory. Deferred event serialization can then
encounter a stale `_path` reference and raise `CablePath.DoesNotExist`.

Refresh stale `_path` references through `PathEndpoint.path` and route
internal callers through that accessor. Update `EventContext` to track
the latest serialization source for coalesced duplicate enqueues, while
eagerly freezing delete-event payloads before row removal.

Also avoid mutating `event_rule.action_data` when merging the event
payload.

Fixes #21498
@pheus pheus force-pushed the 21498-eventrules-in-v452-and-v453-result-in-server-error branch from a398601 to 9440c21 Compare April 2, 2026 22:17
@pheus pheus requested a review from arthanson April 2, 2026 22:18
@arthanson arthanson merged commit b4ee2cf into main Apr 2, 2026
11 checks passed
@pheus pheus deleted the 21498-eventrules-in-v452-and-v453-result-in-server-error branch April 3, 2026 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Eventrules in v4.5.2 and v4.5.3 result in server error

2 participants