Fix issue #23: prevent duplicate diagnostics when external node is already publishing

[bmchalenv]

Fixes #23.

Subscribe to /diagnostics in GreenwaveMonitor to track topic names already published externally. add_topic() now returns an error if a topic's diagnostics are already being published by an external node, preventing duplicate and potentially conflicting diagnostic entries.

[greptile-apps]

Greptile Summary

This PR successfully implements automatic detection and prevention of duplicate diagnostics when external nodes are already publishing for a topic.

Key Changes:

• Subscribes to /diagnostics early in constructor to track externally diagnosed topics
• diagnostics_callback() maintains a set of externally diagnosed topic names, excluding topics already monitored by this node (prevents self-blocking on remove/re-add)
• add_topic() and remove_topic() check against external topics and return clear error messages
• Destructor safely resets diagnostics subscription before clearing internal state
• Comprehensive test coverage for both add and remove scenarios with external diagnostics
• Documentation updated to remove manual warning since prevention is now automatic

Design Decisions:

• Uses "best effort" approach - prevents duplicates when external diagnostics exist before add_topic() is called
• Only tracks topics not currently monitored by this node, allowing clean remove/re-add workflows
• Conservative error handling in both add and remove operations provides clear feedback

Confidence Score: 5/5

• This PR is safe to merge with minimal risk
• The implementation is well-designed and thoroughly tested. The logic correctly prevents duplicate diagnostics while avoiding self-blocking scenarios. The known race condition has been acknowledged by senior developers as acceptable for best-effort prevention. Destructor ordering properly prevents use-after-free issues. All changes are additive and don't break existing functionality.
• No files require special attention

Important Files Changed

Filename	Overview
greenwave_monitor/include/greenwave_monitor.hpp	Added member variables and callback for tracking externally diagnosed topics, updated destructor to safely reset diagnostics subscription
greenwave_monitor/src/greenwave_monitor.cpp	Implemented diagnostics tracking callback, added external topic validation in add_topic() and remove_topic() to prevent duplicate diagnostics
greenwave_monitor/test/test_greenwave_monitor.py	Added comprehensive tests for external diagnostics detection in both add and remove scenarios
docs/DESIGN_AND_IMPLEMENTATION.md	Removed manual warning about external topics since the system now handles this automatically

Sequence Diagram

sequenceDiagram
    participant Ext as External Node
    participant Diag as /diagnostics topic
    participant GW as GreenwaveMonitor
    participant User as Service Caller

    Note over GW: Constructor subscribes to /diagnostics early
    
    Ext->>Diag: Publish DiagnosticArray{status.name=/foo}
    Diag->>GW: diagnostics_callback(msg)
    alt /foo not in greenwave_diagnostics_
        GW->>GW: Add /foo to externally_diagnosed_topics_
    else /foo already monitored by us
        Note over GW: Skip (prevents self-blocking)
    end

    User->>GW: add_topic(/foo)
    alt /foo in externally_diagnosed_topics_
        GW-->>User: Error: Topic is externally monitored
    else /foo not externally diagnosed
        GW->>GW: Create subscription and add to greenwave_diagnostics_
        GW-->>User: Success
    end

Loading

_{Last reviewed commit: a25e74a}

[greptile-apps]

_{4 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}